Skip to content

Commit

Permalink
Parse error falls back to Invalid (#1312)
Browse files Browse the repository at this point in the history 
# Description

This PR makes it so that if at any point in the parsing of sbp messages
the parsing fails, it has the option of falling back to an
InvalidMessage which can then be written to the output. The way this PR
acheives this goal is through some major breaking changes. The idea is
that when errors are thrown throughout the process, we store all that
local information in the constructed error. That way, choosing how to
write the output can become as simple as matching on the thrown error
and then basically writing `SbpMessage::from(error)`. This enables us to
keep the previous behavior of skipping errors or returning on errors
simply by refusing to handle the errors.

`sbp2json`, `json2sbp` & `json2json` now have a new optional arg
`--error_handler` which allows the following options `skip`, `return` or
`ToInvalid`. that skip and return are the equivalent of the false and
true passed to fatal-errors arg which as been removed.

Care was taken to remove some panics in the parsing of SBP which could
cause some types of invalid messages to be lost.

Finally this PR does not do things like try to recover from being passed
in malformed json or given an I/O error. In case of those error, if the
`ToInvalid` option is passed, the code will panic.

Some issues with this PR --
1) it has a lot of breaking changes, and it kind of does a lot. Just
want to make sure people like the direction i went in.
2) the lack of recovering from malformed json means I left json2json to
panic if given ToInvalid as an option.
3) It has no unit tests. I wanted some eyes on this code and proposed
solution before totally getting it over the finish line.

# JIRA Reference

https://swift-nav.atlassian.net/browse/DEVINFRA-1220

---------

Co-authored-by: Jason Mobarak <[email protected]>
Co-authored-by: Patrick Crumley <[email protected]>
Co-authored-by: Patrick Crumley <[email protected]>
Co-authored-by: Jason Mobarak <[email protected]>
  • Loading branch information
@adrian-kong @pcrumley @silverjam
5 people authored Jul 18, 2023
1 parent ac9e865 commit 5f3909c
Show file tree
Hide file tree
Showing 156 changed files with 6,485 additions and 4,241 deletions.
169 changes: 155 additions & 14 deletions generator/sbpg/targets/resources/rust/sbp_messages_mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,27 @@
// WARRANTIES OF MERCHANTABILITY AND/OR FITNESS FOR A PARTICULAR PURPOSE.
//! SBP message definitions.

//****************************************************************************
// Automatically generated from
// 'generator/sbpg/targets/resources/rust/sbp_messages_mod.rs'
//
// Please do not hand edit!
//****************************************************************************/


((*- for m in mods *))
pub mod (((m)));
((*- endfor *))
pub mod unknown;
pub mod invalid;

((*- for m in msgs *))
((*- if m.is_real_message *))
use self::(((m.parent_mod_name)))::(((m.mod_name)))::(((m.msg_name)));
((*- endif *))
((*- endfor *))
use self::unknown::Unknown;
use self::invalid::Invalid;

mod lib {
//! Common imports so we can just `use super::lib::*` in all the message files
Expand Down Expand Up @@ -69,11 +79,11 @@ mod lib {
use lib::*;

/// Common functionality available to all SBP messages.
pub trait SbpMessage: WireFormat + Clone + Sized {
pub trait SbpMessage: WireFormat + Clone {
/// Get the message name.
fn message_name(&self) -> &'static str;
/// Get the message type.
fn message_type(&self) -> u16;
fn message_type(&self) -> Option<u16>;
/// Get the sender_id if it is set.
fn sender_id(&self) -> Option<u16>;
/// Set the sender id.
Expand All @@ -89,6 +99,10 @@ pub trait SbpMessage: WireFormat + Clone + Sized {
fn friendly_name(&self) -> &'static str {
""
}
/// Tells you if the message is valid or if it is not a valid message and may need to be
/// special cased at certain points.
fn is_valid(&self) -> bool;
fn into_valid_msg(self) -> Result<Self, crate::messages::invalid::Invalid>;
}

/// Implemented by messages who's message name and type are known at compile time.
Expand Down Expand Up @@ -130,6 +144,8 @@ pub enum Sbp {
((*- endfor *))
/// Unknown message type
Unknown( Unknown ),
/// Invalid message type.
Invalid( Invalid )
}

#[cfg(feature = "serde_json")]
Expand All @@ -145,13 +161,57 @@ impl<'de> serde::Deserialize<'de> for Sbp {
serde_json::from_value::<(((m.msg_name)))>(value).map(Sbp::(((m.msg_name))) )
},
((*- endfor *))
_ => {
serde_json::from_value::<Unknown>(value).map(Sbp::Unknown)
msg_id @ Some(_) => {
serde_json::from_value::<Unknown>(value)
.map(|msg| Unknown {
msg_id,
..msg
})
.map(Sbp::Unknown)
},
None => {
serde_json::from_value::<Invalid>(value)
.map(|msg| Invalid {
..msg
})
.map(Sbp::Invalid)
}
}.map_err(serde::de::Error::custom)
}
}

#[derive(Debug, Clone)]
pub struct SbpMsgParseError {
/// the message type
pub msg_type: u16,
/// the sender_id
pub sender_id: u16,
/// A vec that just contains the invalid payload bytes
pub invalid_payload: Vec<u8>,
}

impl From<SbpMsgParseError> for PayloadParseError {
fn from(
SbpMsgParseError {
invalid_payload,
..
}: SbpMsgParseError,
) -> Self {
Self {
invalid_payload,
}
}
}

impl std::fmt::Display for SbpMsgParseError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "error parsing payload")
}
}

impl std::error::Error for SbpMsgParseError {}


impl Sbp {
/// Parse a message from given fields.
///
Expand All @@ -176,20 +236,34 @@ impl Sbp {
/// Ok(())
/// }
/// ```
pub fn from_parts<B: bytes::Buf>(msg_type: u16, sender_id: u16, mut payload: B) -> Result<Sbp, PayloadParseError> {
match msg_type {
pub fn from_parts<B: bytes::Buf>(msg_type: u16, sender_id: u16, mut payload: B) -> Result<Sbp, SbpMsgParseError> {
let sbp_msg = match msg_type {
((*- for m in msgs *))
(((m.msg_name)))::MESSAGE_TYPE => {
let mut msg = (((m.msg_name)))::parse(&mut payload)?;
msg.set_sender_id(sender_id);
Ok(Sbp::(((m.msg_name)))(msg))
(((m.msg_name)))::parse(&mut payload)
.map(Sbp::(((m.msg_name))) )
},
((*- endfor *))
_ => {
let mut msg = Unknown::parse(&mut payload)?;
msg_type => {
Unknown::parse(&mut payload)
// keep the msg ID we originally saw
.map(|msg| Unknown { msg_id: Some(msg_type), ..msg })
.map(Sbp::Unknown)
}
};
// Inject sender_id, handle error
match sbp_msg {
Ok(mut msg) => {
msg.set_sender_id(sender_id);
Ok(Sbp::Unknown(msg))
Ok(msg)
}
Err(PayloadParseError {
invalid_payload,
}) => Err(SbpMsgParseError {
msg_type,
sender_id,
invalid_payload,
}),
}
}
}
Expand All @@ -205,10 +279,13 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.message_name()
},
Sbp::Invalid(msg) => {
msg.message_name()
},
}
}

fn message_type(&self) -> u16 {
fn message_type(&self) -> Option<u16> {
match self {
((*- for m in msgs *))
Sbp::(((m.msg_name)))(msg) => {
Expand All @@ -218,6 +295,9 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.message_type()
},
Sbp::Invalid(msg) => {
msg.message_type()
},
}
}

Expand All @@ -231,6 +311,9 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.sender_id()
},
Sbp::Invalid(msg) => {
msg.sender_id()
},
}
}

Expand All @@ -244,6 +327,9 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.set_sender_id(new_id)
},
Sbp::Invalid(msg) => {
msg.set_sender_id(new_id)
},
}
}

Expand All @@ -257,6 +343,9 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.encoded_len()
},
Sbp::Invalid(msg) => {
msg.encoded_len()
},
}
}

Expand All @@ -271,6 +360,9 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.gps_time()
},
Sbp::Invalid(msg) => {
msg.gps_time()
},
}
}

Expand All @@ -284,15 +376,53 @@ impl SbpMessage for Sbp {
Sbp::Unknown(msg) => {
msg.friendly_name()
},
Sbp::Invalid(msg) => {
msg.friendly_name()
},
}
}

fn is_valid(&self) -> bool {
match self {
((*- for m in msgs *))
Sbp::(((m.msg_name)))(msg) => {
msg.is_valid()
},
((*- endfor -*))
Sbp::Unknown(msg) => {
msg.is_valid()
},
Sbp::Invalid(msg) => {
msg.is_valid()
},
}
}
fn into_valid_msg(self) -> Result<Self, crate::messages::invalid::Invalid> {
match self {
((*- for m in msgs *))
Sbp::(((m.msg_name)))(msg) => {
Ok(Sbp::(((m.msg_name)))(msg.into_valid_msg()?))
},
((*- endfor -*))
Sbp::Unknown(msg) => {
Ok(Sbp::Unknown(msg.into_valid_msg()?))
},
Sbp::Invalid(msg) => {
// should never pass
let res = msg.into_valid_msg();
debug_assert!(res.is_err(), "invalid messages may never be valid");
Ok(Sbp::Invalid(res?))
},
}
}

}

impl WireFormat for Sbp {
const MIN_LEN: usize = crate::MAX_FRAME_LEN;

fn parse_unchecked<B: Buf>(_: &mut B) -> Self {
unimplemented!("Sbp must be parsed with Sbp::from_frame");
unimplemented!("Sbp must be parsed with Sbp::from_parts");
}

fn write<B: BufMut>(&self, buf: &mut B) {
Expand All @@ -305,6 +435,9 @@ impl WireFormat for Sbp {
Sbp::Unknown(msg) => {
WireFormat::write(msg, buf)
},
Sbp::Invalid(msg) => {
WireFormat::write(msg, buf)
},
}
}

Expand All @@ -318,6 +451,9 @@ impl WireFormat for Sbp {
Sbp::Unknown(msg) => {
WireFormat::len(msg)
},
Sbp::Invalid(msg) => {
WireFormat::len(msg)
},
}
}
}
Expand All @@ -335,3 +471,8 @@ impl From<Unknown> for Sbp {
Sbp::Unknown(msg)
}
}
impl From<Invalid> for Sbp {
fn from(msg: Invalid) -> Self {
Sbp::Invalid(msg)
}
}
11 changes: 9 additions & 2 deletions generator/sbpg/targets/resources/rust/sbp_messages_template.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ impl SbpMessage for (((m.msg_name))) {
fn message_name(&self) -> &'static str {
<Self as ConcreteMessage>::MESSAGE_NAME
}
fn message_type(&self) -> u16 {
<Self as ConcreteMessage>::MESSAGE_TYPE
fn message_type(&self) -> Option<u16> {
Some(<Self as ConcreteMessage>::MESSAGE_TYPE)
}
fn sender_id(&self) -> Option<u16> {
self.sender_id
Expand All @@ -137,6 +137,13 @@ impl SbpMessage for (((m.msg_name))) {
fn encoded_len(&self) -> usize {
WireFormat::len(self) + crate::HEADER_LEN + crate::CRC_LEN
}
fn is_valid(&self) -> bool {
true
}
fn into_valid_msg(self) -> Result<Self, crate::messages::invalid::Invalid> {
Ok(self)
}

(((m.gps_time_fn)))
}

Expand Down
9 changes: 6 additions & 3 deletions generator/sbpg/targets/resources/rust/test/sbp_tests_template.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ fn test_(((s.suite_name|snake_case)))()
};
match &sbp_msg {
sbp::messages::Sbp::(((t.msg.name|lower_acronyms)))(msg) => {
assert_eq!( msg.message_type(), (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg.message_type());
let msg_type = msg.message_type().unwrap();
assert_eq!( msg_type, (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg_type);
let sender_id = msg.sender_id().unwrap();
assert_eq!(sender_id, (((t.sbp.sender))), "incorrect sender id, expected (((t.sbp.sender))), is {sender_id}");
((*- for f in t.fieldskeys *))(((compare_value( (((f))), (((t.fields[f]))) ))))((*- endfor *))
Expand Down Expand Up @@ -96,7 +97,8 @@ fn test_json2sbp_(((s.suite_name|snake_case)))()
};
match &sbp_msg {
sbp::messages::Sbp::(((t.msg.name|lower_acronyms)))(msg) => {
assert_eq!( msg.message_type(), (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg.message_type());
let msg_type = msg.message_type().unwrap();
assert_eq!( msg_type, (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg_type);
let sender_id = msg.sender_id().unwrap();
assert_eq!(sender_id, (((t.sbp.sender))), "incorrect sender id, expected (((t.sbp.sender))), is {sender_id}");
((*- for f in t.fieldskeys *))(((compare_value( (((f))), (((t.fields[f]))) ))))((*- endfor *))
Expand Down Expand Up @@ -139,7 +141,8 @@ fn test_sbp2json_(((s.suite_name|snake_case)))()
let sbp_msg = sbp::messages::Sbp::(((t.msg.name|lower_acronyms)))(serde_json::from_str(std::str::from_utf8(json_buffer.as_slice()).unwrap().to_string().as_str()).unwrap());
match &sbp_msg {
sbp::messages::Sbp::(((t.msg.name|lower_acronyms)))(msg) => {
assert_eq!( msg.message_type(), (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg.message_type());
let msg_type = msg.message_type().unwrap();
assert_eq!( msg_type, (((t.msg_type))), "Incorrect message type, expected (((t.msg_type))), is {}", msg_type);
let sender_id = msg.sender_id().unwrap();
assert_eq!(sender_id, (((t.sbp.sender))), "incorrect sender id, expected (((t.sbp.sender))), is {sender_id}");
((*- for f in t.fieldskeys *))(((compare_value( (((f))), (((t.fields[f]))) ))))((*- endfor *))
Expand Down
Loading

0 comments on commit 5f3909c

Please sign in to comment.