From fed4bcfd1140a92b137302f7f09aeb38680000b8 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Wed, 12 Jun 2024 17:41:27 -0700 Subject: [PATCH 1/2] Fix rpc error when encoding default is legacy The rpc code was assuming that a default object header will be in flatbuffers format, but at SPI we have changed it to default to legacy until we are ready to adopt flatbuffer format files. Changing the default causes rpc related tests to fail. It is possible that rpc does not handle legacy format files without the change. Since rpc now sends the FlatObject data over the wire as raw bytes, but when using legacy format the header will be set to legacy, the receiving end tries to respect the header and decode the raw bytes as if they are in legacy format, as would be proper behavior when reading a legacy file from disk. But when decoding an rpc message, the payload is _always_ in flatbuffers format. A new constructor was added that disregards the header's claimed format and always treats the payload as flatbuffers encoded. This also means that the server will write an object with a legacy header out to disk in legacy format. Is this the desired behavior? Should `graph::Object::new_with_default_header` be changed to stop assuming what a "default" header is and hard code itself to use the new encoding? It is unclear if the requirements of this method are to produce a header set the new encoding, or to really just respect whatever the default in the code is set to. Signed-off-by: J Robert Ray --- crates/spfs/src/graph/object.rs | 30 ++++++++++++++++++++++++++++ crates/spfs/src/proto/conversions.rs | 7 ++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/crates/spfs/src/graph/object.rs b/crates/spfs/src/graph/object.rs index 5b1f3f9032..f53e745811 100644 --- a/crates/spfs/src/graph/object.rs +++ b/crates/spfs/src/graph/object.rs @@ -113,6 +113,36 @@ impl Object { } } + /// Create an object from the encoded bytes. + /// + /// In memory, objects always use the latest flatbuffer + /// format. The given bytes may be discarded or reconstructed + /// if a conversion is necessary, but the header is preserved + /// in order to ensure that the object does not change it's + /// digest unless explicitly marked to do so. + /// + /// Unlike [`Object::new`] this constructor doesn't respect the header and + /// treats the payload as flatbuffers encoded data. + pub fn new_from_flatbuffers_payload>(buf: B) -> crate::Result { + let bytes = buf.into(); + let header = Header::new(&bytes)?; + let Some(_kind) = header.object_kind() else { + return Err(ObjectError::UnexpectedKind(header.object_kind_number()).into()); + }; + let Some(_format) = header.encoding_format() else { + return Err(ObjectError::UnknownEncoding(header.encoding_format_number()).into()); + }; + // all we need to do with a flatbuffer is validate it, without + // any need to change or reallocate the buffer + flatbuffers::root::(&bytes[Header::SIZE..]) + .map_err(ObjectError::InvalidFlatbuffer)?; + Ok(Object { + buf: bytes, + offset: 0, + _t: PhantomData, + }) + } + /// Constructs a new [`Object`] instance from the provided flatbuffer. /// /// # Safety diff --git a/crates/spfs/src/proto/conversions.rs b/crates/spfs/src/proto/conversions.rs index 71bf8bb42b..07d52464f0 100644 --- a/crates/spfs/src/proto/conversions.rs +++ b/crates/spfs/src/proto/conversions.rs @@ -180,7 +180,12 @@ impl TryFrom for graph::Object { "Unexpected and unsupported object kind {:?}", source.kind ))), - Some(Kind::Buffer(buf)) => graph::Object::new(buf), + Some(Kind::Buffer(buf)) => { + // Object data passed via RPC is always passed in flatbuffers + // format, despite the header possibly claiming to be in + // legacy format. + graph::Object::new_from_flatbuffers_payload(buf) + } None => Err(Error::String( "Expected non-empty object kind in rpc message".to_string(), )), From 7db4831fba50a064d534ff247d7b7e4f6120225f Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Thu, 4 Jul 2024 15:13:13 -0700 Subject: [PATCH 2/2] Fix "it's" vs "its" Signed-off-by: J Robert Ray --- crates/spfs/src/graph/object.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/spfs/src/graph/object.rs b/crates/spfs/src/graph/object.rs index f53e745811..3182308743 100644 --- a/crates/spfs/src/graph/object.rs +++ b/crates/spfs/src/graph/object.rs @@ -60,7 +60,7 @@ impl Object { /// In memory, objects always use the latest flatbuffer /// format. The given bytes may be discarded or reconstructed /// if a conversion is necessary, but the header is preserved - /// in order to ensure that the object does not change it's + /// in order to ensure that the object does not change its /// digest unless explicitly marked to do so. pub fn new>(buf: B) -> crate::Result { let bytes = buf.into(); @@ -118,7 +118,7 @@ impl Object { /// In memory, objects always use the latest flatbuffer /// format. The given bytes may be discarded or reconstructed /// if a conversion is necessary, but the header is preserved - /// in order to ensure that the object does not change it's + /// in order to ensure that the object does not change its /// digest unless explicitly marked to do so. /// /// Unlike [`Object::new`] this constructor doesn't respect the header and