From a4248570556edb2565c2d11b9447dc966aa25560 Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 10:01:08 +1100 Subject: [PATCH 01/13] added error logs --- rust/sbp2json/src/lib.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 9a1c0c725a..56c08e7157 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -102,6 +102,13 @@ where if fatal_errors { Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) } else { - Box::new(messages.filter_map(|m| m.ok())) + Box::new(messages.filter_map(|m| { + if let Ok(m) = m { + Some(m) + } else { + eprintln!("{:?}", m.err()); + None + } + })) } } From 37570224de99f896e64eb2cc5be5da972831eb56 Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 14:12:48 +1100 Subject: [PATCH 02/13] remove take until for inclusiveness --- rust/sbp2json/src/lib.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 56c08e7157..14402c3a17 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -99,16 +99,9 @@ where I: Iterator> + 'a, E: std::error::Error + 'a, { - if fatal_errors { - Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) - } else { - Box::new(messages.filter_map(|m| { - if let Ok(m) = m { - Some(m) - } else { - eprintln!("{:?}", m.err()); - None - } - })) - } + let log_err = move |e| match fatal_errors { + true => panic!("{:?}", e), + false => eprintln!("{e:?}"), + }; + Box::new(messages.filter_map(move |m| m.map_err(log_err).ok())) } From 593f15daaf3ec2fa264cb4c9013726135ffe6cdf Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 14:25:46 +1100 Subject: [PATCH 03/13] if else --- rust/sbp2json/src/lib.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 14402c3a17..80e30ed741 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -99,9 +99,12 @@ where I: Iterator> + 'a, E: std::error::Error + 'a, { - let log_err = move |e| match fatal_errors { - true => panic!("{:?}", e), - false => eprintln!("{e:?}"), + let log_err = move |e| { + if fatal_errors { + panic!("{:?}", e) + } else { + eprintln!("{e:?}") + } }; Box::new(messages.filter_map(move |m| m.map_err(log_err).ok())) } From 8c63cb1f74636cb93730902fe913efa288328abe Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 14:42:50 +1100 Subject: [PATCH 04/13] eprintln error --- rust/sbp2json/src/lib.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 80e30ed741..f97aa81784 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -99,12 +99,9 @@ where I: Iterator> + 'a, E: std::error::Error + 'a, { - let log_err = move |e| { - if fatal_errors { - panic!("{:?}", e) - } else { - eprintln!("{e:?}") - } - }; - Box::new(messages.filter_map(move |m| m.map_err(log_err).ok())) + if fatal_errors { + Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) + } else { + Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e:?}")).ok())) + } } From e5a83a5d2e08301dde7908b7b8d415aded09fc6d Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 15:23:31 +1100 Subject: [PATCH 05/13] terminate on error --- rust/sbp/src/json/de.rs | 11 ++++++++--- rust/sbp2json/src/lib.rs | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index 10cbdcb3e9..b932bbc884 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -10,6 +10,7 @@ use crate::{ messages::Sbp, BUFLEN, }; +use crate::messages::logging::MsgLog; /// Deserialize the IO stream into an iterator of messages. pub fn iter_messages(input: R) -> impl Iterator> { @@ -75,11 +76,15 @@ impl Decoder for JsonDecoder { type Error = JsonError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - let value = match decode_one::(src)? { + let ret = decode_one::(src); + if ret.is_err(){ + return Ok(None); + } + let value = match ret? { Some(v) => v, None => return Ok(None), }; - self.parse_json(value).map(Option::Some) + self.parse_json(value).map(Some) } } @@ -112,6 +117,6 @@ where match value.transpose() { Ok(v) => Ok(v), Err(e) if e.is_eof() => Ok(None), - Err(e) => Err(e), + Err(e) => Err(e), // returns error, on json errors } } diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index f97aa81784..9a1c0c725a 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -102,6 +102,6 @@ where if fatal_errors { Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) } else { - Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e:?}")).ok())) + Box::new(messages.filter_map(|m| m.ok())) } } From 728671cccecf54776584949a082d0daac9759c3a Mon Sep 17 00:00:00 2001 From: adrian-kong <35755741+adrian-kong@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:37:15 +1100 Subject: [PATCH 06/13] lint --- rust/sbp/src/json/de.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index b932bbc884..c4562c3362 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -77,7 +77,7 @@ impl Decoder for JsonDecoder { fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { let ret = decode_one::(src); - if ret.is_err(){ + if ret.is_err() { return Ok(None); } let value = match ret? { From 70907a4b12d1d9311365d42a0206c78fc4793be6 Mon Sep 17 00:00:00 2001 From: adrian-kong <35755741+adrian-kong@users.noreply.github.com> Date: Mon, 21 Nov 2022 16:17:10 +1100 Subject: [PATCH 07/13] remove msglog --- rust/sbp/src/json/de.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index c4562c3362..102f4a2593 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -10,7 +10,6 @@ use crate::{ messages::Sbp, BUFLEN, }; -use crate::messages::logging::MsgLog; /// Deserialize the IO stream into an iterator of messages. pub fn iter_messages(input: R) -> impl Iterator> { From 15ee54e266d2e296279d5d3dd50d028ae8209ceb Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Mon, 21 Nov 2022 16:38:16 +1100 Subject: [PATCH 08/13] print err --- rust/sbp2json/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 9a1c0c725a..f97aa81784 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -102,6 +102,6 @@ where if fatal_errors { Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) } else { - Box::new(messages.filter_map(|m| m.ok())) + Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e:?}")).ok())) } } From 4f2df9efd32bc076307632856580c39870a8f2a5 Mon Sep 17 00:00:00 2001 From: Jason Mobarak Date: Mon, 21 Nov 2022 00:58:18 -0800 Subject: [PATCH 09/13] advance buffer so stream will terminate, log fatal errors too --- rust/sbp/src/json/de.rs | 20 +++++++++++++++----- rust/sbp2json/src/lib.rs | 12 +++++++++++- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index 102f4a2593..dbccc97010 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -76,9 +76,6 @@ impl Decoder for JsonDecoder { fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { let ret = decode_one::(src); - if ret.is_err() { - return Ok(None); - } let value = match ret? { Some(v) => v, None => return Ok(None), @@ -115,7 +112,20 @@ where buf.advance(bytes_read); match value.transpose() { Ok(v) => Ok(v), - Err(e) if e.is_eof() => Ok(None), - Err(e) => Err(e), // returns error, on json errors + // For semantic errors, we need to dump the invalid JSON object + Err(e) if e.is_data() => { + let mut de = Deserializer::from_slice(buf).into_iter::(); + let _ = de.next(); + let bytes_read = de.byte_offset(); + buf.advance(bytes_read); + Err(e) + } + // For syntax (and EOF errors) we dump the whole buffer so the stream + // will terminate. + Err(e) if e.is_syntax() || e.is_eof() => { + buf.advance(buf.len()); + Err(e) + } + Err(e) => Err(e), } } diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index f97aa81784..a0355482c7 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -100,7 +100,17 @@ where E: std::error::Error + 'a, { if fatal_errors { - Box::new(messages.take_while(|m| m.is_ok()).map(|m| m.unwrap())) + Box::new( + messages + .take_while(|m| match &m { + Ok(_) => true, + Err(e) => { + eprintln!("{e:?}"); + false + } + }) + .map(|m| m.unwrap()), + ) } else { Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e:?}")).ok())) } From efd5795c44913351167ea72397665419580a55a9 Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Tue, 22 Nov 2022 10:01:57 +1100 Subject: [PATCH 10/13] more fluent style --- rust/sbp2json/src/lib.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index a0355482c7..50c9a39c9d 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -102,13 +102,7 @@ where if fatal_errors { Box::new( messages - .take_while(|m| match &m { - Ok(_) => true, - Err(e) => { - eprintln!("{e:?}"); - false - } - }) + .take_while(|m| m.as_ref().map_err(|e| eprintln!("{e:?}")).is_ok()) .map(|m| m.unwrap()), ) } else { From 31387c23dd2574c697b22651b2430eafbe01d26d Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Tue, 22 Nov 2022 15:03:13 +1100 Subject: [PATCH 11/13] terminate on eof fix test --- rust/sbp/src/json/de.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index dbccc97010..0b5eda1609 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -75,8 +75,7 @@ impl Decoder for JsonDecoder { type Error = JsonError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - let ret = decode_one::(src); - let value = match ret? { + let value = match decode_one::(src)? { Some(v) => v, None => return Ok(None), }; @@ -112,6 +111,7 @@ where buf.advance(bytes_read); match value.transpose() { Ok(v) => Ok(v), + Err(e) if e.is_eof() => Ok(None), // For semantic errors, we need to dump the invalid JSON object Err(e) if e.is_data() => { let mut de = Deserializer::from_slice(buf).into_iter::(); @@ -122,7 +122,7 @@ where } // For syntax (and EOF errors) we dump the whole buffer so the stream // will terminate. - Err(e) if e.is_syntax() || e.is_eof() => { + Err(e) if e.is_syntax() => { buf.advance(buf.len()); Err(e) } From 3628d09baf2d4955b89019280aeb0dde3a517b24 Mon Sep 17 00:00:00 2001 From: Jason Mobarak Date: Tue, 22 Nov 2022 18:00:18 +1100 Subject: [PATCH 12/13] Error out if two consecutive EOF errors occur --- rust/sbp/src/json/de.rs | 32 +++++++++++++++++++++++++------- rust/sbp2json/src/lib.rs | 4 ++-- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/rust/sbp/src/json/de.rs b/rust/sbp/src/json/de.rs index 0b5eda1609..865f7deb0d 100644 --- a/rust/sbp/src/json/de.rs +++ b/rust/sbp/src/json/de.rs @@ -44,12 +44,14 @@ pub fn stream_messages( #[derive(Debug, Default)] struct JsonDecoder { payload_buf: Vec, + eof_seen: bool, } impl JsonDecoder { fn new() -> Self { JsonDecoder { payload_buf: Vec::with_capacity(BUFLEN), + eof_seen: false, } } @@ -75,7 +77,7 @@ impl Decoder for JsonDecoder { type Error = JsonError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - let value = match decode_one::(src)? { + let value = match decode_one::(&mut self.eof_seen, src)? { Some(v) => v, None => return Ok(None), }; @@ -84,11 +86,13 @@ impl Decoder for JsonDecoder { } #[derive(Debug)] -struct Json2JsonDecoder; +struct Json2JsonDecoder { + eof_seen: bool, +} impl Json2JsonDecoder { fn framed(input: R) -> FramedRead { - FramedRead::new(input, Self) + FramedRead::new(input, Self { eof_seen: false }) } } @@ -97,11 +101,11 @@ impl Decoder for Json2JsonDecoder { type Error = JsonError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - decode_one::(src).map_err(Into::into) + decode_one::(&mut self.eof_seen, src).map_err(Into::into) } } -fn decode_one(buf: &mut BytesMut) -> Result, serde_json::Error> +fn decode_one(eof_seen: &mut bool, buf: &mut BytesMut) -> Result, serde_json::Error> where T: DeserializeOwned, { @@ -110,8 +114,22 @@ where let bytes_read = de.byte_offset(); buf.advance(bytes_read); match value.transpose() { - Ok(v) => Ok(v), - Err(e) if e.is_eof() => Ok(None), + Ok(v) => { + *eof_seen = false; + Ok(v) + } + // One EOF error is OK, if we fail to resolve it on a second read then we return an error. + // (Note this will incorrectly fail if there is a JSON object that spans the underlying buffer of dencode, + // which is 8k, see https://github.com/swift-nav/dencode/blob/df889f926013085212af55d0e31d59fa71f16833/src/framed_read.rs#L10) + Err(e) if e.is_eof() => { + if *eof_seen { + buf.advance(buf.len()); + Err(e) + } else { + *eof_seen = true; + Ok(None) + } + } // For semantic errors, we need to dump the invalid JSON object Err(e) if e.is_data() => { let mut de = Deserializer::from_slice(buf).into_iter::(); diff --git a/rust/sbp2json/src/lib.rs b/rust/sbp2json/src/lib.rs index 50c9a39c9d..5d6bd21159 100644 --- a/rust/sbp2json/src/lib.rs +++ b/rust/sbp2json/src/lib.rs @@ -102,10 +102,10 @@ where if fatal_errors { Box::new( messages - .take_while(|m| m.as_ref().map_err(|e| eprintln!("{e:?}")).is_ok()) + .take_while(|m| m.as_ref().map_err(|e| eprintln!("{e}")).is_ok()) .map(|m| m.unwrap()), ) } else { - Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e:?}")).ok())) + Box::new(messages.filter_map(|m| m.map_err(|e| eprintln!("{e}")).ok())) } } From d17370cad052017dd4156cf629966b695f26fbdc Mon Sep 17 00:00:00 2001 From: adrian-kong Date: Wed, 23 Nov 2022 10:03:42 +1100 Subject: [PATCH 13/13] Adds some testing for json converters --- rust/sbp2json/tests/common/mod.rs | 7 +++++++ rust/sbp2json/tests/test_error.rs | 27 +++++++++++++++++++++++++++ test_data/errors/error_eof.json | 2 ++ test_data/errors/error_untagged.json | 1 + 4 files changed, 37 insertions(+) create mode 100644 rust/sbp2json/tests/test_error.rs create mode 100644 test_data/errors/error_eof.json create mode 100644 test_data/errors/error_untagged.json diff --git a/rust/sbp2json/tests/common/mod.rs b/rust/sbp2json/tests/common/mod.rs index 9018f8969a..1fe379a43a 100644 --- a/rust/sbp2json/tests/common/mod.rs +++ b/rust/sbp2json/tests/common/mod.rs @@ -257,3 +257,10 @@ fn json_file_equals(a: File, b: File) -> bool { true } } + +/// Returns relative data path +pub fn data_path(relative_path: &str) -> PathBuf { + let root = find_project_root().unwrap(); + let root = root.as_path(); + root.join(relative_path) +} diff --git a/rust/sbp2json/tests/test_error.rs b/rust/sbp2json/tests/test_error.rs new file mode 100644 index 0000000000..0a178e615d --- /dev/null +++ b/rust/sbp2json/tests/test_error.rs @@ -0,0 +1,27 @@ +mod common; + +use crate::common::{data_path, find_project_root}; +use assert_cmd::cargo::CommandCargoExt; +use assert_cmd::Command; + +/// Asserts termination of json decoder on broken inputs +#[test] +fn test_json_converters_termination() { + let input_path = data_path("test_data/errors/error_eof.json"); + let mut cmd = Command::cargo_bin("json2sbp").unwrap(); + cmd.arg(input_path) + .assert() + .success() + .stderr("expected `,` or `}` at line 1 column 185\n"); +} + +/// Asserts error for invalid msg_type +#[test] +fn test_json_untagged() { + let input_path = data_path("test_data/errors/error_untagged.json"); + let mut cmd = Command::cargo_bin("json2sbp").unwrap(); + cmd.arg(input_path) + .assert() + .success() + .stderr("data did not match any variant of untagged enum JsonInput\n"); +} diff --git a/test_data/errors/error_eof.json b/test_data/errors/error_eof.json new file mode 100644 index 0000000000..86926fd872 --- /dev/null +++ b/test_data/errors/error_eof.json @@ -0,0 +1,2 @@ +{"crc":10555,"length":17,"msg_type":2304,"payload":"xYZjQJhhAWwAYhDy/3v+DgA=","preamble":85,"sender":51228,"tow":1080264389,"tow_f":152,"acc_x":353,"acc_y":108,"acc_z":4194,"gyr_x":114{ +{"crc":10555,"length":17,"msg_type":2304,"payload":"xYZjQJhhAWwAYhDy/3v+DgA=","preamble":85,"sender":51228,"tow":1080264389,"tow_f":152,"acc_x":353,"acc_y":108,"acc_z":4194,"gyr_x":114,"gyr_y":-389,"gyr_z":14} diff --git a/test_data/errors/error_untagged.json b/test_data/errors/error_untagged.json new file mode 100644 index 0000000000..a87dad481b --- /dev/null +++ b/test_data/errors/error_untagged.json @@ -0,0 +1 @@ +{"crc":10555,"length":17,"msg_type":"2304","payload":"xYZjQJhhAWwAYhDy/3v+DgA=","preamble":85,"sender":51228,"tow":1080264389,"tow_f":152,"acc_x":353,"acc_y":108,"acc_z":4194,"gyr_x":114,"gyr_y":-389,"gyr_z":14}