Skip to content

Commit 24c9612

Browse files
bakeyclaude
andcommitted
fix(sources): make the nested non-finite guard real — capture Json pins as YAML, convert fallibly
Review finding on r3710042428: first_non_finite was dead code. By the time it ran, serde_json's f64 visitor had already mapped a nested .nan/.inf to Value::Null during the untagged Json variant's deserialization (Number::from_f64(...).map_or(Value::Null, …) in serde_json 1.0.150), so a nested non-finite silently pinned {"threshold": null} — exactly the silent-null-at-query-time failure the guard's own comment promised to prevent — and the guard's Some arm was unreachable by construction. FixedValueDoc::Json now captures serde_yaml::Value, where the non-finite is still observable, and converts through a fallible yaml_to_json that rejects everything JSON cannot spell: non-finite floats at any depth, non-string mapping keys, and YAML tags (the last already rejected upstream by serde's untagged buffering — pinned by a test — and kept as defense in depth). first_non_finite is deleted; into_fixed folds into the single convert_table match so no arm is unreachable. Regression tests cover nested .nan in an object, .inf in a nested list, a numeric mapping key, the tag rejection message, and the pass side (nested finite floats/bools/nulls convert faithfully, asserted on to_json output). Also tightens the users cursor e2e per r3710042432: each request now asserts its EXACT input key set ([pageSize] page 1, [pageSize, startCursor] page 2), the same nothing-extra-on-the-wire guard the search test applies. Full suite: cargo test -p skardi --lib → 818 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent f86160f commit 24c9612

2 files changed

Lines changed: 193 additions & 42 deletions

File tree

crates/skardi/src/sources/providers/open_connector/packs/loader.rs

Lines changed: 176 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -106,19 +106,32 @@ fn convert_table(
106106
// YAML happily parses `.nan` / `.inf` / `-.inf`, but JSON has no
107107
// spelling for them — FixedValue::to_json would silently send null
108108
// at query time, bypassing the startup diagnostic this loader
109-
// promises. Finite-only, enforced here.
110-
let non_finite = match &value {
111-
FixedValueDoc::Float(v) => (!v.is_finite()).then(|| v.to_string()),
112-
FixedValueDoc::Json(v) => first_non_finite(v),
113-
_ => None,
109+
// promises. Finite-only, enforced here. The nested case CANNOT be
110+
// checked after the fact on a `serde_json::Value`: serde_json's
111+
// f64 visitor maps a non-finite to `Value::Null` during
112+
// deserialization (`Number::from_f64(...).map_or(Value::Null, …)`),
113+
// so the loss has already happened by then — which is why the
114+
// `Json` variant captures `serde_yaml::Value` and converts here,
115+
// where the non-finite is still observable.
116+
let fixed = match value {
117+
FixedValueDoc::Float(v) if !v.is_finite() => {
118+
return Err(format!(
119+
"{id}: fixed input '{key}' contains {v}, which has no JSON \
120+
spelling; pin finite numbers only"
121+
));
122+
}
123+
FixedValueDoc::Bool(v) => FixedValue::Bool(v),
124+
FixedValueDoc::Int(v) => FixedValue::Int(v),
125+
FixedValueDoc::Float(v) => FixedValue::Float(v),
126+
FixedValueDoc::Str(v) => FixedValue::Str(leak_str(v)),
127+
FixedValueDoc::StrList(v) => FixedValue::StrList(leak_str_slice(v)),
128+
FixedValueDoc::Json(v) => {
129+
let json = yaml_to_json(v)
130+
.map_err(|reason| format!("{id}: fixed input '{key}' {reason}"))?;
131+
FixedValue::Json(Box::leak(Box::new(json)))
132+
}
114133
};
115-
if let Some(shown) = non_finite {
116-
return Err(format!(
117-
"{id}: fixed input '{key}' contains {shown}, which has no JSON \
118-
spelling; pin finite numbers only"
119-
));
120-
}
121-
fixed_inputs.push((leak_str(key), value.into_fixed()));
134+
fixed_inputs.push((leak_str(key), fixed));
122135
}
123136
let table = SourcePackTable {
124137
id,
@@ -337,21 +350,6 @@ fn convert_filter(doc: FilterDoc) -> FilterMapping {
337350
}
338351
}
339352

340-
/// First non-finite float inside a JSON value, if any. serde_json parses
341-
/// no non-finite literals itself, but serde_yaml's `.nan`/`.inf` arrive
342-
/// through the untagged `Json` variant as f64s that `Number::from_f64`
343-
/// silently turns into null on write — the same trap as bare floats.
344-
fn first_non_finite(value: &serde_json::Value) -> Option<String> {
345-
match value {
346-
serde_json::Value::Number(n) => {
347-
n.as_f64().filter(|f| !f.is_finite()).map(|f| f.to_string())
348-
}
349-
serde_json::Value::Array(items) => items.iter().find_map(first_non_finite),
350-
serde_json::Value::Object(map) => map.values().find_map(first_non_finite),
351-
_ => None,
352-
}
353-
}
354-
355353
fn leak_str(s: String) -> &'static str {
356354
Box::leak(s.into_boxed_str())
357355
}
@@ -510,22 +508,67 @@ enum FixedValueDoc {
510508
Str(String),
511509
StrList(Vec<String>),
512510
/// Object-shaped inputs (e.g. Notion's search `filter`). Captured as a
513-
/// JSON value so nesting is unrestricted; converted with the same
514-
/// finite-floats guard as the scalar variants.
515-
Json(serde_json::Value),
511+
/// YAML value — NOT `serde_json::Value`, whose f64 visitor converts a
512+
/// nested `.nan`/`.inf` to `Value::Null` during deserialization,
513+
/// destroying the evidence before any guard can run — and converted
514+
/// fallibly by [`yaml_to_json`] at the call site, where non-finite
515+
/// floats, non-string mapping keys, and YAML tags are rejected with
516+
/// targeted messages.
517+
Json(serde_yaml::Value),
516518
}
517519

518-
impl FixedValueDoc {
519-
fn into_fixed(self) -> FixedValue {
520-
match self {
521-
Self::Bool(v) => FixedValue::Bool(v),
522-
Self::Int(v) => FixedValue::Int(v),
523-
Self::Float(v) => FixedValue::Float(v),
524-
Self::Str(v) => FixedValue::Str(leak_str(v)),
525-
Self::StrList(v) => FixedValue::StrList(leak_str_slice(v)),
526-
Self::Json(v) => FixedValue::Json(Box::leak(Box::new(v))),
520+
/// Convert a YAML value to JSON, rejecting everything JSON cannot spell:
521+
/// non-finite floats (`.nan` / `.inf` / `-.inf`, which `serde_json` would
522+
/// silently write as `null`), non-string mapping keys, and YAML tags. The
523+
/// error is a reason fragment; callers prefix the table/key identity.
524+
fn yaml_to_json(value: serde_yaml::Value) -> Result<serde_json::Value, String> {
525+
use serde_yaml::Value as Yaml;
526+
Ok(match value {
527+
Yaml::Null => serde_json::Value::Null,
528+
Yaml::Bool(b) => serde_json::Value::from(b),
529+
Yaml::Number(n) => {
530+
if let Some(i) = n.as_i64() {
531+
serde_json::Value::from(i)
532+
} else if let Some(u) = n.as_u64() {
533+
serde_json::Value::from(u)
534+
} else {
535+
let f = n.as_f64().unwrap_or(f64::NAN);
536+
if !f.is_finite() {
537+
return Err(format!(
538+
"contains {f}, which has no JSON spelling; pin finite numbers only"
539+
));
540+
}
541+
serde_json::Value::from(f)
542+
}
527543
}
528-
}
544+
Yaml::String(s) => serde_json::Value::from(s),
545+
Yaml::Sequence(items) => serde_json::Value::Array(
546+
items
547+
.into_iter()
548+
.map(yaml_to_json)
549+
.collect::<Result<_, _>>()?,
550+
),
551+
Yaml::Mapping(map) => {
552+
let mut out = serde_json::Map::with_capacity(map.len());
553+
for (k, v) in map {
554+
let Yaml::String(k) = k else {
555+
return Err(
556+
"contains a non-string mapping key, which JSON cannot represent"
557+
.to_string(),
558+
);
559+
};
560+
out.insert(k, yaml_to_json(v)?);
561+
}
562+
serde_json::Value::Object(out)
563+
}
564+
// Unreachable through the untagged FixedValueDoc path (serde's
565+
// untagged buffering rejects tags during deserialization, pinned by
566+
// the loader test), kept as defense in depth for any future direct
567+
// caller.
568+
Yaml::Tagged(_) => {
569+
return Err("contains a YAML tag, which JSON cannot represent".to_string());
570+
}
571+
})
529572
}
530573

531574
#[derive(Deserialize)]
@@ -593,6 +636,43 @@ mod tests {
593636
}
594637
}
595638

639+
/// The pass side of the nested-value conversion: a finite nested float
640+
/// (and the rest of the JSON scalar set) survives the YAML→JSON
641+
/// conversion faithfully — proving the strict rejection above is a
642+
/// guard, not a ban on nesting.
643+
#[test]
644+
fn nested_finite_values_in_a_json_pin_convert_faithfully() {
645+
let pack = parse_pack(
646+
r#"kind: pack
647+
pack: demo
648+
version: 1
649+
tables:
650+
things:
651+
action: demo.list
652+
row_path: "$.items"
653+
pagination: { strategy: page_number, page_input: page, page_size_input: perPage, page_size: 10 }
654+
fixed_inputs:
655+
filter:
656+
threshold: 1.5
657+
flags: [true, 2, "three"]
658+
inner: { level: null }
659+
columns:
660+
- { name: id, path: id, type: uint64, nullable: false }
661+
"#,
662+
)
663+
.expect("nested finite values are legal");
664+
let (key, value) = &pack.tables[0].fixed_inputs[0];
665+
assert_eq!(*key, "filter");
666+
assert_eq!(
667+
value.to_json(),
668+
serde_json::json!({
669+
"threshold": 1.5,
670+
"flags": [true, 2, "three"],
671+
"inner": { "level": null }
672+
})
673+
);
674+
}
675+
596676
#[test]
597677
fn misspelled_keys_fail_loudly() {
598678
// deny_unknown_fields end to end: a typo'd pagination key must not
@@ -808,6 +888,62 @@ tables:
808888
- { name: id, path: id, type: uint64, nullable: false }"#,
809889
"no JSON spelling",
810890
),
891+
// NESTED non-finites, through the untagged Json variant. These
892+
// are the regression for the dead first_non_finite guard: a
893+
// `serde_json::Value` capture had already converted the nested
894+
// `.nan` to null before any check could run (serde_json's f64
895+
// visitor maps non-finite to Value::Null), so the pin silently
896+
// became `{"threshold": null}`. The YAML capture keeps the
897+
// non-finite observable and the conversion rejects it.
898+
(
899+
r#" action: demo.list
900+
row_path: "$.items"
901+
pagination: { strategy: page_number, page_input: page, page_size_input: perPage, page_size: 10 }
902+
fixed_inputs:
903+
filter:
904+
threshold: .nan
905+
columns:
906+
- { name: id, path: id, type: uint64, nullable: false }"#,
907+
"no JSON spelling",
908+
),
909+
(
910+
r#" action: demo.list
911+
row_path: "$.items"
912+
pagination: { strategy: page_number, page_input: page, page_size_input: perPage, page_size: 10 }
913+
fixed_inputs:
914+
filter:
915+
bounds: [1.5, .inf]
916+
columns:
917+
- { name: id, path: id, type: uint64, nullable: false }"#,
918+
"no JSON spelling",
919+
),
920+
// The other two YAML shapes JSON cannot spell, same variant.
921+
(
922+
r#" action: demo.list
923+
row_path: "$.items"
924+
pagination: { strategy: page_number, page_input: page, page_size_input: perPage, page_size: 10 }
925+
fixed_inputs:
926+
filter:
927+
1: numeric-key
928+
columns:
929+
- { name: id, path: id, type: uint64, nullable: false }"#,
930+
"non-string mapping key",
931+
),
932+
(
933+
r#" action: demo.list
934+
row_path: "$.items"
935+
pagination: { strategy: page_number, page_input: page, page_size_input: perPage, page_size: 10 }
936+
fixed_inputs:
937+
filter:
938+
payload: !custom tagged
939+
columns:
940+
- { name: id, path: id, type: uint64, nullable: false }"#,
941+
// Rejected before yaml_to_json ever runs: serde's untagged
942+
// buffering cannot represent a YAML tag, so deserialization
943+
// itself fails — the Tagged arm in yaml_to_json is defense
944+
// in depth behind this.
945+
"do not support enum input",
946+
),
811947
(
812948
r#" action: demo.list
813949
row_path: "$.items"

crates/skardi/src/sources/providers/open_connector/packs/notion.rs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -511,10 +511,25 @@ bindings:
511511

512512
let inputs = execute_inputs(&gateway);
513513
assert_eq!(inputs.len(), 2, "two cursor pages");
514-
assert!(inputs[0].get("startCursor").is_none(), "{}", inputs[0]);
515514
assert_eq!(inputs[1]["startCursor"], "cur-2");
516-
for input in &inputs {
515+
for (page, (input, expected_keys)) in inputs
516+
.iter()
517+
.zip([vec!["pageSize"], vec!["pageSize", "startCursor"]])
518+
.enumerate()
519+
{
517520
assert_eq!(input["pageSize"], 100, "page-size hint: {input}");
521+
// Exactly the declared inputs, nothing else — the same
522+
// nothing-extra-on-the-wire guard the search test applies, so
523+
// users' wire declaration is pinned in full, absence included
524+
// (page 1 carries no cursor).
525+
let mut keys: Vec<&str> = input
526+
.as_object()
527+
.expect("input object")
528+
.keys()
529+
.map(String::as_str)
530+
.collect();
531+
keys.sort_unstable();
532+
assert_eq!(keys, expected_keys, "page {} input keys", page + 1);
518533
}
519534
}
520535

0 commit comments

Comments
 (0)