refactor: commonize MessagePack (rmp) helpers - #3665
Conversation
markovejnovic
changed the title
markovejnovic
changed the title
markovejnovic
changed the title
Greptile SummaryCentralizes MessagePack encode/decode helpers from six scattered copy-pastas into
Confidence Score: 4/5Safe to merge — serialization format is unchanged and all callers produce and consume identical bytes. The change touches serialization/deserialization paths across eight crates. No format changes, logic is preserved, and new proptest round-trips provide good coverage. The sole note is an omitted expect_eof in ScriptRecord::Delete that pre-dates this PR and is not a regression. crates/atuin-scripts/src/store/record.rs — the Delete arm does not call expect_eof, unlike the parallel HistoryRecord::Delete path updated in this PR. Important Files Changed
Reviews (1): Last reviewed commit: "more cleanup" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 1 => { | ||
| let bytes = bytes.remaining_slice(); | ||
| let (id, _) = decode::read_str_from_slice(bytes).map_err(error_report)?; | ||
| Ok(ScriptRecord::Delete(Uuid::parse_str(id)?)) | ||
| let id = atu_rmp::decode::read_string(&mut bytes).decode()?; | ||
| Ok(ScriptRecord::Delete(Uuid::parse_str(&id)?)) | ||
| } |
There was a problem hiding this comment.
HistoryRecord::Delete path in history/store.rs was updated in this PR to call expect_eof, but the equivalent ScriptRecord::Delete arm here was not. A truncated or padded payload would silently succeed as long as the UUID is valid. The old code also dropped trailing bytes with let (id, _) = ..., so this isn't a regression — but it's inconsistent with the treatment of the other Delete arms in this PR.
taylordotfish
left a comment
taylordotfish
left a comment
There was a problem hiding this comment.
not sure why a bunch of comments were changed, imo most are worse now
also prefer type parameters and where clauses to impl Trait, not enough to block a PR but it's weird that they were changed
also not sure why a bunch of the functions take Bytes instead of a generic RmpRead or RmpWrite, I think they were better before
diff is also larger than necessary, imo this PR changes a bunch of things that didn't need changing
| pub fn write_optional<W: RmpWrite, T>( | ||
| out: &mut W, | ||
| value: Option<T>, | ||
| write: impl FnOnce(&mut W, T) -> Result<(), EncodeError<W::Error>>, | ||
| ) -> Result<(), EncodeError<W::Error>> { | ||
| match value { | ||
| Some(v) => write(out, v), | ||
| None => rmp::encode::write_nil(out) | ||
| .map_err(|e| EncodeError::from(ValueWriteError::InvalidMarkerWrite(e))), | ||
| } | ||
| } |
There was a problem hiding this comment.
can you keep this function the way it was before?
| pub fn write_u8<W: RmpWrite>(out: &mut W, val: u8) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_u8(out, val)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_u16<W: RmpWrite>(out: &mut W, val: u16) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_u16(out, val)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_u64<W: RmpWrite>(out: &mut W, val: u64) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_u64(out, val)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_uint<W: RmpWrite>(out: &mut W, val: u64) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_uint(out, val)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_sint<W: RmpWrite>(out: &mut W, val: i64) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_sint(out, val)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_bool<W: RmpWrite>(out: &mut W, val: bool) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_bool(out, val) | ||
| .map_err(|e| EncodeError::from(ValueWriteError::InvalidMarkerWrite(e))) | ||
| } | ||
|
|
||
| pub fn write_str<W: RmpWrite>(out: &mut W, data: &str) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_str(out, data)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_bin<W: RmpWrite>(out: &mut W, data: &[u8]) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_bin(out, data)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_array_len<W: RmpWrite>(out: &mut W, len: u32) -> Result<(), EncodeError<W::Error>> { | ||
| rmp::encode::write_array_len(out, len)?; | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
can we generate these with a macro instead?
| /// Read an owned [`String`]. | ||
| pub fn read_string<'a>( | ||
| bytes: &mut Bytes<'a>, | ||
| ) -> Result<String, DecodeStringError<'a, BytesReadError>> { | ||
| let slice = bytes.remaining_slice(); | ||
| let (string, rest) = match mp::read_str_from_slice(slice) { | ||
| Ok(pair) => pair, | ||
| Err(e) => { | ||
| if let DecodeStringError::TypeMismatch(_) = e { | ||
| // rmp consumes the marker byte on a type mismatch; match that so | ||
| // `read_optional` can detect nil. | ||
| bytes | ||
| .read_u8() | ||
| .expect("TypeMismatch implies the stream contains a marker byte"); | ||
| } | ||
| return Err(e); | ||
| } | ||
| }; | ||
| *bytes = Bytes::new(rest); | ||
| Ok(string.into()) | ||
| } | ||
|
|
||
| /// Read a value that may be nil, returning [`None`] for nil. | ||
| pub fn read_optional<'a, T, E>( | ||
| bytes: &mut Bytes<'a>, | ||
| read: impl FnOnce(&mut Bytes<'a>) -> Result<T, E>, | ||
| ) -> Result<Option<T>, DecodeError<'a>> | ||
| where | ||
| E: Into<DecodeError<'a>>, | ||
| { | ||
| match read(bytes) { | ||
| Ok(v) => Ok(Some(v)), | ||
| Err(e) => { | ||
| let e = e.into(); | ||
| if let Some(Marker::Null) = e.type_mismatch() { | ||
| Ok(None) | ||
| } else { | ||
| Err(e) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we keep these functions the way they were before?
| atu_rmp::encode::write_u16(&mut output, Version::LATEST.as_int())?; | ||
| atu_rmp::encode::write_array_len(&mut output, Version::LATEST.min_fields())?; | ||
|
|
||
| atu_rmp::encode::write_str(&mut output, &self.id.0)?; | ||
| atu_rmp::encode::write_u64(&mut output, self.timestamp.unix_timestamp_nanos() as u64)?; | ||
| atu_rmp::encode::write_sint(&mut output, self.duration)?; | ||
| atu_rmp::encode::write_sint(&mut output, self.exit)?; | ||
| atu_rmp::encode::write_str(&mut output, &self.command)?; | ||
| atu_rmp::encode::write_str(&mut output, &self.cwd)?; | ||
| atu_rmp::encode::write_str(&mut output, &self.session)?; | ||
| atu_rmp::encode::write_str(&mut output, &self.hostname)?; | ||
|
|
||
| atu_rmp::encode::write_optional( |
There was a problem hiding this comment.
can we import encode and decode like before to minimize the diff?
2 participants
This PR commonizes some code regarding
rmp^(de)?serializers$.atuin-scriptsno longer panics on deserialize failures.error_reportcopy-pastas, now there's oneDecodeError.Downside is:
rmp::andatuin_common::rmp::