From d2db893ac74441071921787c72c311c139e0887c Mon Sep 17 00:00:00 2001 From: Sean Curtis Date: Wed, 8 Jan 2025 10:07:41 -0800 Subject: [PATCH] Allow serialization for MemoryFile The MemoryFile's contents are serialized as a std::vector to account for the fact that std::string can include arbitrary byte values. (In yaml, serializing std::vector> produces a base64-encoded octet stream with the tag "!!binary".) --- common/BUILD.bazel | 1 + common/memory_file.h | 42 +++++++++++++++++++++++++++++---- common/test/file_source_test.cc | 10 ++++---- common/test/memory_file_test.cc | 28 +++++++++++++++++----- 4 files changed, 66 insertions(+), 15 deletions(-) diff --git a/common/BUILD.bazel b/common/BUILD.bazel index b6d4b9c90fa2..037fc7ff426c 100644 --- a/common/BUILD.bazel +++ b/common/BUILD.bazel @@ -201,6 +201,7 @@ drake_cc_library( hdrs = ["memory_file.h"], deps = [ ":essential", + ":name_value", ":reset_after_move", ":sha256", ], diff --git a/common/memory_file.h b/common/memory_file.h index 14af989570cc..82d1a310d4d9 100644 --- a/common/memory_file.h +++ b/common/memory_file.h @@ -2,9 +2,11 @@ #include #include +#include #include "drake/common/drake_copyable.h" #include "drake/common/fmt.h" +#include "drake/common/name_value.h" #include "drake/common/reset_after_move.h" #include "drake/common/sha256.h" @@ -67,14 +69,44 @@ class MemoryFile final { any number less than or equal to zero. */ std::string to_string(int contents_limit = 100) const; - /** Serialization stub. + /** Passes this object to an Archive. - %MemoryFile cannot actually be serialized yet. Attempting to do will throw. - This stub merely permits FileSource to be serialized (when it contains a - `std::filesystem::path`). */ + When used in yaml, it is important to specify _all_ fields. Applications may + depend on the `extension` value to determine what to do with the file + contents. Omitting `extension` would make it unusable in those cases. + + Omitting `filename_hint` is less dangerous; error messages would lack a + helpful identifier, but things would otherwise function. + + The value of contents should be a base64-encoded string of the file contents. + Yaml's `!!binary` tag is required to declare the value is such a string. + Serializing the %MemoryFile will produce such a string. Writing a yaml file + by hand will be more challenging. + + The following yaml would produce a %MemoryFile with contents equal to: + + This is an example of memory file test contents. + + ```yaml + contents: !!binary VGhpcyBpcyBhbiBleGFtcGxlIG9mIG1 + lbW9yeSBmaWxlIHRlc3QgY29udGVudHMu + extension: .txt + filename_hint: payload.txt + ``` + */ template void Serialize(Archive* a) { - throw std::runtime_error("Serialization for MemoryFile not yet supported."); + // vector get serialized to !!binary yaml values. We don't know if + // we're reading or writing, so we'll mindlessly convert to and from a byte + // string. + auto* data = reinterpret_cast(contents_.value().data()); + std::vector bytes(data, data + contents_.value().size()); + a->Visit(MakeNameValue("contents", &bytes)); + contents_ = + std::string(reinterpret_cast(bytes.data()), bytes.size()); + + a->Visit(MakeNameValue("extension", &extension_.value())); + a->Visit(MakeNameValue("filename_hint", &filename_hint_.value())); } private: diff --git a/common/test/file_source_test.cc b/common/test/file_source_test.cc index 64da86ce42cc..67663ca2678e 100644 --- a/common/test/file_source_test.cc +++ b/common/test/file_source_test.cc @@ -47,12 +47,14 @@ GTEST_TEST(FileSourceTest, SerializePath) { EXPECT_EQ(std::get(dut.source), std::get(decoded.source)); } -/* The MemoryFile value simply throws (see MemoryFile implementation). */ +/* The MemoryFile value gets (de)serialized. */ GTEST_TEST(FileSourceTest, SerializeMemoryFile) { const HasFileSource dut{.source = MemoryFile("stuff", ".ext", "hint")}; - DRAKE_EXPECT_THROWS_MESSAGE( - yaml::SaveYamlString(dut), - "Serialization for MemoryFile not yet supported."); + const std::string y = yaml::SaveYamlString(dut); + const auto decoded = yaml::LoadYamlString(y); + ASSERT_TRUE(std::holds_alternative(decoded.source)); + EXPECT_EQ(std::get(dut.source).contents(), + std::get(decoded.source).contents()); } } // namespace diff --git a/common/test/memory_file_test.cc b/common/test/memory_file_test.cc index c153bd79a9a6..1d340c5bfce4 100644 --- a/common/test/memory_file_test.cc +++ b/common/test/memory_file_test.cc @@ -111,12 +111,28 @@ GTEST_TEST(MemoryFileTest, ToString) { EXPECT_THAT(fmt::to_string(file), testing::HasSubstr("\"0123456789\"")); } -/* Serialization compiles but throws. */ -GTEST_TEST(MemoryFileTest, SerializationThrows) { - const MemoryFile dut("stuff", ".ext", "hint"); - DRAKE_EXPECT_THROWS_MESSAGE( - yaml::SaveYamlString(dut), - "Serialization for MemoryFile not yet supported."); +/** Confirm that this can be serialized appropriately. The serialization work + (with all of its nuances) get tested elsewhere. Here, we're simply testing + that the fields get serialized and deserialized as expected. */ +GTEST_TEST(MemoryFileTest, Serialization) { + // This content is the text shown in the serialization documentation. Same + // for the extension and filename hint as well. + const std::string content("This is an example of memory file test contents."); + const std::string content_b64( + "VGhpcyBpcyBhbiBleGFtcGxlIG9mIG1lbW9yeSBmaWxlIHRlc3QgY29udGVudHMu"); + const MemoryFile dut(content, ".txt", "payload.txt"); + + // Serialization. + const std::string y = yaml::SaveYamlString(dut); + EXPECT_EQ(y, fmt::format("contents: !!binary {}\nextension: " + ".txt\nfilename_hint: payload.txt\n", + content_b64)); + + // Deserialization. + const auto from_yaml = yaml::LoadYamlString(y); + EXPECT_EQ(from_yaml.contents(), dut.contents()); + EXPECT_EQ(from_yaml.extension(), dut.extension()); + EXPECT_EQ(from_yaml.filename_hint(), dut.filename_hint()); } } // namespace