Skip to content

Commit 28f9996

Browse files
moore-ryanmoore-ryan
authored
fix: Fix panic during CSV import when encountering invalid floats (#433)
* fix: Fix panic on 'Nan' in csv_str_to_value This fixes the panic that was thrown when csv_str_to_value attempted to convert an 'invalid' float value into a JSON number. If the float value was not representable as a JSON number, it would panic. The new behaviour is to fall back to representing 'invalid' float values as Strings, but still attempt to parse them as numbers first. * chore: Update 'log' dep version to 0.4.18 Updating the version of the 'log' dependency to 0.4.18 to fix compilation issues on recent Nightly toolchains. See async-rs/async-std#1058 for a similar issue. * chore: Run rustfmt, add clippy allow Ran rustfmt, there were some existing changes that ended up getting reformatted. Added an explicit `#[allow(clippy::default_constructed_unit_structs)]` in `core/src/graph/number.rs` - `OsRng::default()` is used in a unit test.
1 parent 5c26f2b commit 28f9996

File tree

6 files changed

+191
-57
lines changed

6 files changed

+191
-57
lines changed

‎Cargo.lock

Lines changed: 89 additions & 19 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎core/src/graph/number.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,7 @@ pub mod test {
495495
overflowed: false,
496496
};
497497

498+
#[allow(clippy::default_constructed_unit_structs)]
498499
let mut rng = OsRng::default();
499500

500501
for i in 0..255 {

‎core/src/schema/content/number.rs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -531,9 +531,7 @@ impl RangeStep<u64> {
531531
impl Categorical<u64> {
532532
pub fn upcast(self, to: NumberContentKind) -> Result<NumberContent> {
533533
match to {
534-
NumberContentKind::U64 => {
535-
Ok(number_content::U64::Categorical(self).into())
536-
}
534+
NumberContentKind::U64 => Ok(number_content::U64::Categorical(self).into()),
537535
NumberContentKind::I64 => {
538536
let cast = Categorical {
539537
seen: self
@@ -543,12 +541,15 @@ impl Categorical<u64> {
543541
i64::try_from(k)
544542
.map(|k_cast| (k_cast, v))
545543
.map_err(|err| err.into())
546-
}).collect::<Result<_>>()?,
544+
})
545+
.collect::<Result<_>>()?,
547546
total: self.total,
548547
};
549548
Ok(number_content::I64::Categorical(cast).into())
550549
}
551-
NumberContentKind::F64 => Err(failed!(target: Release, "cannot upcast categorical subtypes to accept floats; try changing this another numerical subtype manually"))
550+
NumberContentKind::F64 => Err(
551+
failed!(target: Release, "cannot upcast categorical subtypes to accept floats; try changing this another numerical subtype manually"),
552+
),
552553
}
553554
}
554555
}
@@ -611,7 +612,9 @@ impl Categorical<i64> {
611612
Err(failed!(target: Release, "cannot downcast numerical subtypes"))
612613
}
613614
NumberContentKind::I64 => Ok(number_content::I64::Categorical(self).into()),
614-
NumberContentKind::F64 => Err(failed!(target: Release, "cannot upcast categorical subtypes to accept floats; try changing this another numerical subtype manually")),
615+
NumberContentKind::F64 => Err(
616+
failed!(target: Release, "cannot upcast categorical subtypes to accept floats; try changing this another numerical subtype manually"),
617+
),
615618
}
616619
}
617620
}

‎core/src/schema/optionalise.rs

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -21,32 +21,37 @@ impl OptionaliseApi for Namespace {
2121
fn optionalise(&mut self, optionalise: Optionalise) -> Result<()> {
2222
let target = optionalise.at;
2323
match target.parent() {
24-
Some(parent) => {
25-
match self.get_s_node_mut(&parent)? {
26-
Content::Object(object_content) |
27-
Content::Array(ArrayContent { content: box Content::Object(object_content), .. }) => {
28-
let fc = object_content.get_mut(&target.last())?;
29-
let owned = std::mem::replace(fc, Content::null());
30-
*fc = if optionalise.optional {
31-
owned.into_nullable()
32-
} else if owned.is_nullable() {
33-
match owned {
34-
Content::OneOf(OneOfContent { variants }) => {
35-
variants.into_iter().map(|vc| *vc.content).find(|v| !v.is_null()).unwrap()
36-
}
37-
_ => unreachable!()
38-
}
39-
} else {
40-
owned
41-
};
42-
Ok(())
43-
}
44-
otherwise => Err(failed!(target: Release, "Only fields of objects can be optional. But the reference '{}' (whose parent is '{}') is contained in a content node of type '{}'.", target, parent, otherwise.kind()))
24+
Some(parent) => match self.get_s_node_mut(&parent)? {
25+
Content::Object(object_content)
26+
| Content::Array(ArrayContent {
27+
content: box Content::Object(object_content),
28+
..
29+
}) => {
30+
let fc = object_content.get_mut(&target.last())?;
31+
let owned = std::mem::replace(fc, Content::null());
32+
*fc = if optionalise.optional {
33+
owned.into_nullable()
34+
} else if owned.is_nullable() {
35+
match owned {
36+
Content::OneOf(OneOfContent { variants }) => variants
37+
.into_iter()
38+
.map(|vc| *vc.content)
39+
.find(|v| !v.is_null())
40+
.unwrap(),
41+
_ => unreachable!(),
42+
}
43+
} else {
44+
owned
45+
};
46+
Ok(())
4547
}
46-
}
47-
None => {
48-
Err(failed!(target: Release, "Field '{}' is top-level (meaning it just references a collection). Top-level fields cannot be made optional.", target))
49-
}
48+
otherwise => Err(
49+
failed!(target: Release, "Only fields of objects can be optional. But the reference '{}' (whose parent is '{}') is contained in a content node of type '{}'.", target, parent, otherwise.kind()),
50+
),
51+
},
52+
None => Err(
53+
failed!(target: Release, "Field '{}' is top-level (meaning it just references a collection). Top-level fields cannot be made optional.", target),
54+
),
5055
}
5156
}
5257
}

‎synth/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ anyhow = "1.0.32"
4242

4343
structopt = "0.3.18"
4444

45-
log = "0.4.11"
45+
log = "0.4.18"
4646
env_logger = "0.7.1"
4747

4848
num_cpus = "1.0"

‎synth/src/cli/csv/mod.rs

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -238,17 +238,33 @@ fn csv_str_to_value(s: &str) -> serde_json::Value {
238238
serde_json::Value::Bool(true)
239239
} else if s == "false" {
240240
serde_json::Value::Bool(false)
241-
} else if let Ok(unsigned) = s.trim().parse::<u64>() {
242-
serde_json::Value::Number(serde_json::Number::from(unsigned))
243-
} else if let Ok(signed) = s.trim().parse::<i64>() {
244-
serde_json::Value::Number(serde_json::Number::from(signed))
245-
} else if let Ok(float) = s.trim().parse::<f64>() {
246-
serde_json::Value::Number(serde_json::Number::from_f64(float).unwrap())
241+
} else if let Some(number) = csv_str_to_json_number(s) {
242+
number
247243
} else {
248244
serde_json::Value::String(s.to_string())
249245
}
250246
}
251247

248+
fn csv_str_to_json_number(s: &str) -> Option<serde_json::Value> {
249+
let trimmed = s.trim();
250+
251+
if let Ok(unsigned) = trimmed.parse::<u64>() {
252+
Some(serde_json::Value::Number(serde_json::Number::from(
253+
unsigned,
254+
)))
255+
} else if let Ok(signed) = trimmed.parse::<i64>() {
256+
Some(serde_json::Value::Number(serde_json::Number::from(signed)))
257+
} else {
258+
// Certain float values, such as NaN and (+/-)Inf are not valid JSON Numbers,
259+
// so ensure that we handle failures here
260+
trimmed
261+
.parse::<f64>()
262+
.ok()
263+
.and_then(serde_json::Number::from_f64)
264+
.map(serde_json::Value::Number)
265+
}
266+
}
267+
252268
#[derive(Debug, PartialEq)]
253269
pub enum CsvOutput {
254270
Namespace(Vec<(String, String)>),
@@ -550,5 +566,44 @@ mod tests {
550566
assert_eq!(csv_str_to_value("64"), serde_json::json!(64));
551567
assert_eq!(csv_str_to_value("-64"), serde_json::json!(-64));
552568
assert_eq!(csv_str_to_value("64.1"), serde_json::json!(64.1));
569+
570+
assert_eq!(
571+
csv_str_to_value("Nan"),
572+
serde_json::Value::String("Nan".to_string())
573+
);
574+
assert_eq!(
575+
csv_str_to_value("NaN"),
576+
serde_json::Value::String("NaN".to_string())
577+
);
578+
assert_eq!(
579+
csv_str_to_value("Inf"),
580+
serde_json::Value::String("Inf".to_string())
581+
);
582+
assert_eq!(
583+
csv_str_to_value("-Inf"),
584+
serde_json::Value::String("-Inf".to_string())
585+
);
586+
}
587+
588+
#[test]
589+
fn test_csv_str_to_json_number() {
590+
assert_eq!(csv_str_to_json_number("64"), Some(serde_json::json!(64)));
591+
assert_eq!(csv_str_to_json_number("-64"), Some(serde_json::json!(-64)));
592+
assert_eq!(
593+
csv_str_to_json_number("64.1"),
594+
Some(serde_json::json!(64.1))
595+
);
596+
597+
assert_eq!(csv_str_to_json_number("0"), Some(serde_json::json!(0)));
598+
assert_eq!(csv_str_to_json_number("-0"), Some(serde_json::json!(0)));
599+
600+
assert_eq!(csv_str_to_json_number("true"), None);
601+
assert_eq!(csv_str_to_json_number("false"), None);
602+
assert_eq!(csv_str_to_json_number(""), None);
603+
604+
assert_eq!(csv_str_to_json_number("Nan"), None);
605+
assert_eq!(csv_str_to_json_number("NaN"), None);
606+
assert_eq!(csv_str_to_json_number("Inf"), None);
607+
assert_eq!(csv_str_to_json_number("-Inf"), None);
553608
}
554609
}

0 commit comments

Comments
 (0)