Change Compact Binary Package API to represent binary attachments as compressed buffers identified by their raw hash.

Change Compact Binary Package serialization for binary attachments to compressed buffers and objects as objects followed by their hash.

Ported changes from Zousar's pending CL17372417

1 files changed, 257 insertions, 161 deletions

diff --git a/zencore/compactbinarypackage.cpp b/zencore/compactbinarypackage.cpp
index 7880164f9..a345f2b1b 100644
--- a/zencore/compactbinarypackage.cpp
+++ b/zencore/compactbinarypackage.cpp
@@ -16,22 +16,47 @@ CbAttachment::CbAttachment(const CompressedBuffer& InValue) : CbAttachment(InVal
 {
 }
 
-CbAttachment::CbAttachment(CompressedBuffer&& InValue)
+CbAttachment::CbAttachment(const SharedBuffer& InValue) : CbAttachment(CompositeBuffer(InValue))
 {
-	Value.emplace<CompressedBuffer>(std::move(InValue).MakeOwned());
 }
 
-CbAttachment::CbAttachment(const SharedBuffer& InValue)
+CbAttachment::CbAttachment(const SharedBuffer& InValue, [[maybe_unused]] const IoHash& InHash)
 : CbAttachment(InValue.IsNull() ? CompressedBuffer()
 								: CompressedBuffer::Compress(InValue, OodleCompressor::NotSet, OodleCompressionLevel::None))
 {
+	// This could be more efficient, and should at the very least try to validate the hash
 }
 
-CbAttachment::CbAttachment(const SharedBuffer& InValue, [[maybe_unused]] const IoHash& InHash)
-: CbAttachment(InValue.IsNull() ? CompressedBuffer()
-								: CompressedBuffer::Compress(InValue, OodleCompressor::NotSet, OodleCompressionLevel::None))
+CbAttachment::CbAttachment(const CompositeBuffer& InValue) : Value{std::in_place_type<BinaryValue>, InValue}
 {
-	// This could be more efficient, and should at the very least try to validate the hash
+	if (std::get<BinaryValue>(Value).Buffer.IsNull())
+	{
+		Value.emplace<nullptr_t>();
+	}
+}
+
+CbAttachment::CbAttachment(CompositeBuffer&& InValue) : Value{std::in_place_type<BinaryValue>, InValue}
+{
+	if (std::get<BinaryValue>(Value).Buffer.IsNull())
+	{
+		Value.emplace<nullptr_t>();
+	}
+}
+
+CbAttachment::CbAttachment(CompositeBuffer&& InValue, const IoHash& Hash) : Value{std::in_place_type<BinaryValue>, InValue, Hash}
+{
+	if (std::get<BinaryValue>(Value).Buffer.IsNull())
+	{
+		Value.emplace<nullptr_t>();
+	}
+}
+
+CbAttachment::CbAttachment(CompressedBuffer&& InValue) : Value(std::in_place_type<CompressedBuffer>, InValue)
+{
+	if (std::get<CompressedBuffer>(Value).IsNull())
+	{
+		Value.emplace<nullptr_t>();
+	}
 }
 
 CbAttachment::CbAttachment(const CbObject& InValue, const IoHash* const InHash)
@@ -70,114 +95,139 @@ CbAttachment::TryLoad(IoBuffer& InBuffer, BufferAllocator Allocator)
 bool
 CbAttachment::TryLoad(CbFieldIterator& Fields)
 {
-	const CbObjectView ObjectView = Fields.AsObjectView();
-	if (Fields.HasError())
+	if (const CbObjectView ObjectView = Fields.AsObjectView(); !Fields.HasError())
+	{
+		// Is a null object or object not prefixed with a precomputed hash value
+		Value.emplace<CbObjectValue>(CbObject(ObjectView, Fields.GetOuterBuffer()), ObjectView.GetHash());
+		++Fields;
+	}
+	else if (const IoHash ObjectAttachmentHash = Fields.AsObjectAttachment(); !Fields.HasError())
+	{
+		// Is an object
+		++Fields;
+		const CbObjectView InnerObjectView = Fields.AsObjectView();
+		if (Fields.HasError())
+		{
+			return false;
+		}
+		Value.emplace<CbObjectValue>(CbObject(InnerObjectView, Fields.GetOuterBuffer()), ObjectAttachmentHash);
+		++Fields;
+	}
+	else if (const IoHash BinaryAttachmentHash = Fields.AsBinaryAttachment(); !Fields.HasError())
+	{
+		// Is an uncompressed binary blob
+		++Fields;
+		MemoryView BinaryView = Fields.AsBinaryView();
+		if (Fields.HasError())
+		{
+			return false;
+		}
+		Value.emplace<BinaryValue>(SharedBuffer::MakeView(BinaryView, Fields.GetOuterBuffer()), BinaryAttachmentHash);
+		++Fields;
+	}
+	else if (MemoryView BinaryView = Fields.AsBinaryView(); !Fields.HasError())
 	{
-		// Is a buffer
-		const MemoryView BinaryView = Fields.AsBinaryView();
 		if (BinaryView.GetSize() > 0)
 		{
+			// Is a compressed binary blob
 			Value.emplace<CompressedBuffer>(
 				CompressedBuffer::FromCompressed(SharedBuffer::MakeView(BinaryView, Fields.GetOuterBuffer())).MakeOwned());
-
 			++Fields;
 		}
 		else
 		{
+			// Is an uncompressed empty binary blob
+			Value.emplace<BinaryValue>(SharedBuffer::MakeView(BinaryView, Fields.GetOuterBuffer()), IoHash::HashBuffer(nullptr, 0));
 			++Fields;
-			Value.emplace<CompressedBuffer>();
 		}
 	}
 	else
 	{
-		// It's an object
-		++Fields;
-		IoHash Hash;
-		if (ObjectView)
-		{
-			Hash = Fields.AsObjectAttachment();
-			++Fields;
-		}
-		else
-		{
-			Hash = IoHash::HashBuffer(MemoryView{});
-		}
-		Value.emplace<CbObjectValue>(CbObject(ObjectView, Fields->GetOuterBuffer()), Hash);
+		return false;
 	}
 
 	return true;
 }
 
-bool
-CbAttachment::TryLoad(BinaryReader& Reader, BufferAllocator Allocator)
+static bool
+TryLoad_ArchiveFieldIntoAttachment(CbAttachment& TargetAttachment, CbField&& Field, BinaryReader& Reader, BufferAllocator Allocator)
 {
-	CbField			   Field	  = LoadCompactBinary(Reader, Allocator);
-	const CbObjectView ObjectView = Field.AsObjectView();
-
-	if (Field.HasError())
+	if (const CbObjectView ObjectView = Field.AsObjectView(); !Field.HasError())
 	{
-		// It's a buffer
-		const MemoryView BinaryView = Field.AsBinaryView();
-		if (BinaryView.GetSize() > 0)
+		// Is a null object or object not prefixed with a precomputed hash value
+		TargetAttachment = CbAttachment(CbObject(ObjectView, std::move(Field)), ObjectView.GetHash());
+	}
+	else if (const IoHash ObjectAttachmentHash = Field.AsObjectAttachment(); !Field.HasError())
+	{
+		// Is an object
+		Field = LoadCompactBinary(Reader, Allocator);
+		if (!Field.IsObject())
 		{
-			Value.emplace<CompressedBuffer>(
-				CompressedBuffer::FromCompressed(SharedBuffer::MakeView(BinaryView, Field.GetOuterBuffer())).MakeOwned());
+			return false;
 		}
-		else
+		TargetAttachment = CbAttachment(std::move(Field).AsObject(), ObjectAttachmentHash);
+	}
+	else if (const IoHash BinaryAttachmentHash = Field.AsBinaryAttachment(); !Field.HasError())
+	{
+		// Is an uncompressed binary blob
+		Field				= LoadCompactBinary(Reader, Allocator);
+		SharedBuffer Buffer = Field.AsBinary();
+		if (Field.HasError())
 		{
-			Value.emplace<CompressedBuffer>();
+			return false;
 		}
+		TargetAttachment = CbAttachment(CompositeBuffer(Buffer), BinaryAttachmentHash);
 	}
-	else
+	else if (SharedBuffer Buffer = Field.AsBinary(); !Field.HasError())
 	{
-		// It's an object
-		IoHash Hash;
-		if (ObjectView)
+		if (Buffer.GetSize() > 0)
 		{
-			std::vector<uint8_t> HashBuffer;
-			CbField				 HashField = LoadCompactBinary(Reader, [&HashBuffer](uint64_t Size) -> UniqueBuffer {
-				 HashBuffer.resize(Size);
-				 return UniqueBuffer::MakeMutableView(HashBuffer.data(), Size);
-			 });
-			Hash						   = HashField.AsAttachment();
-			if (HashField.HasError() || ObjectView.GetHash() != Hash)
-			{
-				// Error
-				return false;
-			}
+			// Is a compressed binary blob
+			TargetAttachment = CbAttachment(CompressedBuffer::FromCompressed(std::move(Buffer)));
 		}
 		else
 		{
-			Hash = IoHash::HashBuffer(MemoryView());
+			// Is an uncompressed empty binary blob
+			TargetAttachment = CbAttachment(CompositeBuffer(Buffer), IoHash::HashBuffer(nullptr, 0));
 		}
-		Value.emplace<CbObjectValue>(CbObject(ObjectView, Field.GetOuterBuffer()), Hash);
+	}
+	else
+	{
+		return false;
 	}
 
 	return true;
 }
 
+bool
+CbAttachment::TryLoad(BinaryReader& Reader, BufferAllocator Allocator)
+{
+	CbField Field = LoadCompactBinary(Reader, Allocator);
+	return TryLoad_ArchiveFieldIntoAttachment(*this, std::move(Field), Reader, Allocator);
+}
+
 void
 CbAttachment::Save(CbWriter& Writer) const
 {
-	if (const CbObjectValue* ObjectValue = std::get_if<CbObjectValue>(&Value))
+	if (const CbObjectValue* ObjValue = std::get_if<CbObjectValue>(&Value))
 	{
-		Writer.AddObject(ObjectValue->Object);
-		if (ObjectValue->Object)
+		if (ObjValue->Object)
 		{
-			Writer.AddObjectAttachment(ObjectValue->Hash);
+			Writer.AddObjectAttachment(ObjValue->Hash);
 		}
+		Writer.AddObject(ObjValue->Object);
 	}
-	else
+	else if (const BinaryValue* BinValue = std::get_if<BinaryValue>(&Value))
 	{
-		const CompressedBuffer& BufferValue = std::get<CompressedBuffer>(Value);
-		if (BufferValue.GetRawSize())
-		{
-			Writer.AddBinary(BufferValue.GetCompressed());
-		}
-		else  // Null
+		if (BinValue->Buffer.GetSize() > 0)
 		{
-			Writer.AddBinary(MemoryView());
+			Writer.AddBinaryAttachment(BinValue->Hash);
 		}
+		Writer.AddBinary(BinValue->Buffer);
+	}
+	else if (const CompressedBuffer* BufferValue = std::get_if<CompressedBuffer>(&Value))
+	{
+		Writer.AddBinary(BufferValue->GetCompressed());
 	}
 }
 
@@ -192,14 +242,19 @@ CbAttachment::Save(BinaryWriter& Writer) const
 bool
 CbAttachment::IsNull() const
 {
-	if (const CompressedBuffer* Buffer = std::get_if<CompressedBuffer>(&Value))
-	{
-		return Buffer->IsNull();
-	}
-	else
-	{
-		return false;
-	}
+	return std::holds_alternative<nullptr_t>(Value);
+}
+
+bool
+CbAttachment::IsBinary() const
+{
+	return std::holds_alternative<BinaryValue>(Value);
+}
+
+bool
+CbAttachment::IsCompressedBinary() const
+{
+	return std::holds_alternative<CompressedBuffer>(Value);
 }
 
 bool
@@ -213,25 +268,42 @@ CbAttachment::GetHash() const
 {
 	if (const CompressedBuffer* Buffer = std::get_if<CompressedBuffer>(&Value))
 	{
-		return Buffer->IsNull() ? IoHash::HashBuffer(MemoryView()) : IoHash::FromBLAKE3(Buffer->GetRawHash());
+		return IoHash::FromBLAKE3(Buffer->GetRawHash());
 	}
-	else
+
+	if (const BinaryValue* BinValue = std::get_if<BinaryValue>(&Value))
+	{
+		return BinValue->Hash;
+	}
+
+	if (const CbObjectValue* ObjectValue = std::get_if<CbObjectValue>(&Value))
 	{
-		return std::get<CbObjectValue>(Value).Hash;
+		return ObjectValue->Hash;
 	}
+
+	return IoHash::Zero;
 }
 
-SharedBuffer
-CbAttachment::AsBinary() const
+CompositeBuffer
+CbAttachment::AsCompositeBinary() const
 {
-	if (const CompressedBuffer* Buffer = std::get_if<CompressedBuffer>(&Value))
+	if (const BinaryValue* BinValue = std::get_if<BinaryValue>(&Value))
 	{
-		return Buffer->Decompress();
+		return BinValue->Buffer;
 	}
-	else
+
+	return CompositeBuffer::Null;
+}
+
+SharedBuffer
+CbAttachment::AsBinary() const
+{
+	if (const BinaryValue* BinValue = std::get_if<BinaryValue>(&Value))
 	{
-		return std::get<CbObjectValue>(Value).Object.GetBuffer();
+		return BinValue->Buffer.Flatten();
 	}
+
+	return {};
 }
 
 CompressedBuffer
@@ -241,12 +313,8 @@ CbAttachment::AsCompressedBinary() const
 	{
 		return *Buffer;
 	}
-	else
-	{
-		return CompressedBuffer::Compress(std::get<CbObjectValue>(Value).Object.GetBuffer(),
-										  OodleCompressor::NotSet,
-										  OodleCompressionLevel::None);
-	}
+
+	return CompressedBuffer::Null;
 }
 
 /** Access the attachment as compact binary. Defaults to a field iterator with no value on error. */
@@ -257,10 +325,8 @@ CbAttachment::AsObject() const
 	{
 		return ObjectValue->Object;
 	}
-	else
-	{
-		return {};
-	}
+
+	return {};
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -301,10 +367,7 @@ CbPackage::AddAttachment(const CbAttachment& Attachment, AttachmentResolver* Res
 		if (It != Attachments.end() && *It == Attachment)
 		{
 			CbAttachment& Existing = *It;
-			if (Attachment.IsObject() && !Existing.IsObject())
-			{
-				Existing = CbAttachment(CbObject(Existing.AsBinary()), Existing.GetHash());
-			}
+			Existing			   = Attachment;
 		}
 		else
 		{
@@ -358,7 +421,7 @@ CbPackage::GatherAttachments(const CbObject& Value, AttachmentResolver Resolver)
 			}
 			else
 			{
-				AddAttachment(CbAttachment(std::move(Buffer), Hash));
+				AddAttachment(CbAttachment(std::move(Buffer)));
 			}
 		}
 	});
@@ -377,6 +440,7 @@ bool
 CbPackage::TryLoad(CbFieldIterator& Fields)
 {
 	*this = CbPackage();
+
 	while (Fields)
 	{
 		if (Fields.IsNull())
@@ -384,43 +448,76 @@ CbPackage::TryLoad(CbFieldIterator& Fields)
 			++Fields;
 			break;
 		}
-		else if (Fields.IsBinary())
-		{
-			CbAttachment Attachment;
-			Attachment.TryLoad(Fields);
-			AddAttachment(Attachment);
-		}
-		else
+		else if (IoHash Hash = Fields.AsHash(); !Fields.HasError() && !Fields.IsAttachment())
 		{
-			Object = Fields.AsObject();
-			if (Fields->HasError())
+			++Fields;
+			CbObjectView ObjectView = Fields.AsObjectView();
+			if (Fields.HasError() || Hash != ObjectView.GetHash())
 			{
 				return false;
 			}
+			Object = CbObject(ObjectView, Fields.GetOuterBuffer());
 			Object.MakeOwned();
+			ObjectHash = Hash;
 			++Fields;
-			if (Object.CreateIterator())
-			{
-				ObjectHash = Fields.AsObjectAttachment();
-				if (Fields.HasError())
-				{
-					return false;
-				}
-				++Fields;
-			}
-			else
+		}
+		else
+		{
+			CbAttachment Attachment;
+			if (!Attachment.TryLoad(Fields))
 			{
-				Object.Reset();
+				return false;
 			}
+			AddAttachment(Attachment);
 		}
 	}
-
 	return true;
 }
 
 bool
 CbPackage::TryLoad(BinaryReader& Reader, BufferAllocator Allocator, AttachmentResolver* Mapper)
 {
+	// TODO: this needs to re-grow the ability to accept a reference to an attachment which is
+	// not embedded
+
+	ZEN_UNUSED(Mapper);
+
+#if 1
+	*this = CbPackage();
+	for (;;)
+	{
+		CbField Field = LoadCompactBinary(Reader, Allocator);
+		if (!Field)
+		{
+			return false;
+		}
+
+		if (Field.IsNull())
+		{
+			return true;
+		}
+		else if (IoHash Hash = Field.AsHash(); !Field.HasError() && !Field.IsAttachment())
+		{
+			Field					= LoadCompactBinary(Reader, Allocator);
+			CbObjectView ObjectView = Field.AsObjectView();
+			if (Field.HasError() || Hash != ObjectView.GetHash())
+			{
+				return false;
+			}
+			Object	   = CbObject(ObjectView, Field.GetOuterBuffer());
+			ObjectHash = Hash;
+		}
+		else
+		{
+			CbAttachment Attachment;
+			if (!TryLoad_ArchiveFieldIntoAttachment(Attachment, std::move(Field), Reader, Allocator))
+			{
+				return false;
+			}
+			AddAttachment(Attachment);
+		}
+	}
+#else
 	uint8_t	   StackBuffer[64];
 	const auto StackAllocator = [&Allocator, &StackBuffer](uint64_t Size) -> UniqueBuffer {
 		if (Size <= sizeof(StackBuffer))
@@ -494,6 +591,7 @@ CbPackage::TryLoad(BinaryReader& Reader, BufferAllocator Allocator, AttachmentRe
 			}
 		}
 	}
+#endif
 }
 
 void
@@ -501,8 +599,8 @@ CbPackage::Save(CbWriter& Writer) const
 {
 	if (Object)
 	{
+		Writer.AddHash(ObjectHash);
 		Writer.AddObject(Object);
-		Writer.AddObjectAttachment(ObjectHash);
 	}
 	for (const CbAttachment& Attachment : Attachments)
 	{
@@ -567,8 +665,7 @@ TEST_CASE("usonpackage")
 		CHECK_FALSE(bool(Attachment.AsObject()));
 		CHECK_FALSE(Attachment.IsBinary());
 		CHECK_FALSE(Attachment.IsObject());
-		CHECK(Attachment.GetHash() == IoHash::HashBuffer({}));
-		TestSaveLoadValidate("Null", Attachment);
+		CHECK(Attachment.GetHash() == IoHash::Zero);
 	}
 
 	SUBCASE("Binary Attachment")
@@ -596,12 +693,12 @@ TEST_CASE("usonpackage")
 
 		CHECK_FALSE(Attachment.IsNull());
 		CHECK(bool(Attachment));
-		CHECK(Attachment.AsBinary() == Object.GetBuffer());
+		CHECK(Attachment.AsBinary() == SharedBuffer());
 		CHECK(Attachment.AsObject().Equals(Object));
-		CHECK(Attachment.IsBinary());
+		CHECK_FALSE(Attachment.IsBinary());
 		CHECK(Attachment.IsObject());
 		CHECK(Attachment.GetHash() == Object.GetHash());
-		TestSaveLoadValidate("CompactBinary", Attachment);
+		TestSaveLoadValidate("Object", Attachment);
 	}
 
 	SUBCASE("Binary View")
@@ -633,7 +730,7 @@ TEST_CASE("usonpackage")
 
 		CHECK(Attachment.AsBinary() != ObjectView.GetBuffer());
 		CHECK(Attachment.AsObject().Equals(Object));
-		CHECK(Attachment.IsBinary());
+		CHECK_FALSE(Attachment.IsBinary());
 		CHECK(Attachment.IsObject());
 		CHECK(Attachment.GetHash() == IoHash(Object.GetHash()));
 	}
@@ -677,15 +774,14 @@ TEST_CASE("usonpackage")
 		CbFieldIterator FieldsView = CbFieldIterator::MakeRangeView(CbFieldViewIterator(Fields));
 
 		Attachment.TryLoad(FieldsView);
+		MemoryView View;
 
 		CHECK_FALSE(Attachment.IsNull());
 		CHECK(bool(Attachment));
-
-		CHECK(Attachment.AsBinary().GetView().EqualBytes(Value.GetView()));
-		CHECK_FALSE(FieldsView.GetBuffer().GetView().Contains(Attachment.AsObject().GetBuffer().GetView()));
-		CHECK(Attachment.IsBinary());
+		CHECK(Attachment.AsBinary().GetView().EqualBytes(MemoryView()));
+		CHECK_FALSE((!Attachment.AsObject().TryGetSerializedView(View) || FieldsView.GetOuterBuffer().GetView().Contains(View)));
+		CHECK_FALSE(Attachment.IsBinary());
 		CHECK(Attachment.IsObject());
-
 		CHECK(Attachment.GetHash() == Value.GetHash());
 	}
 
@@ -696,7 +792,7 @@ TEST_CASE("usonpackage")
 		CHECK(Attachment.IsNull());
 		CHECK_FALSE(Attachment.IsBinary());
 		CHECK_FALSE(Attachment.IsObject());
-		CHECK(Attachment.GetHash() == IoHash::HashBuffer(SharedBuffer{}));
+		CHECK(Attachment.GetHash() == IoHash::Zero);
 	}
 
 	SUBCASE("Binary Empty")
@@ -714,7 +810,7 @@ TEST_CASE("usonpackage")
 		const CbAttachment Attachment(CbObject{});
 
 		CHECK_FALSE(Attachment.IsNull());
-		CHECK(Attachment.IsBinary());
+		CHECK_FALSE(Attachment.IsBinary());
 		CHECK(Attachment.IsObject());
 		CHECK(Attachment.GetHash() == CbObject().GetHash());
 	}
@@ -833,17 +929,16 @@ TEST_CASE("usonpackage.serialization")
 		CHECK((Object1Attachment && Object1Attachment->AsObject().Equals(Object1)));
 		CHECK((Object2Attachment && Object2Attachment->AsBinary() == Object2.GetBuffer()));
 
-		Package.AddAttachment(CbAttachment(SharedBuffer::Clone(Object1.GetView())));
+		SharedBuffer Object1ClonedBuffer = SharedBuffer::Clone(Object1.GetOuterBuffer());
+		Package.AddAttachment(CbAttachment(Object1ClonedBuffer));
 		Package.AddAttachment(CbAttachment(CbObject::Clone(Object2)));
 
 		CHECK(Package.GetAttachments().size() == 2);
 		CHECK(Package.FindAttachment(Object1.GetHash()) == Object1Attachment);
 		CHECK(Package.FindAttachment(Object2.GetHash()) == Object2Attachment);
 
-		CHECK((Object1Attachment && Object1Attachment->AsObject().Equals(Object1)));
-		CHECK((Object1Attachment && Object1Attachment->AsBinary() == Object1.GetBuffer()));
+		CHECK((Object1Attachment && Object1Attachment->AsBinary() == Object1ClonedBuffer));
 		CHECK((Object2Attachment && Object2Attachment->AsObject().Equals(Object2)));
-		CHECK((Object2Attachment && Object2Attachment->AsBinary() == Object2.GetBuffer()));
 
 		CHECK(std::is_sorted(begin(Package.GetAttachments()), end(Package.GetAttachments())));
 	}
@@ -884,8 +979,8 @@ TEST_CASE("usonpackage.serialization")
 	const IoHash Level1Hash = Level1.GetHash();
 
 	const auto Resolver = [&Level2, &Level2Hash, &Level3, &Level3Hash, &Level4, &Level4Hash](const IoHash& Hash) -> SharedBuffer {
-		return Hash == Level2Hash	? Level2.GetBuffer()
-			   : Hash == Level3Hash ? Level3.GetBuffer()
+		return Hash == Level2Hash	? Level2.GetOuterBuffer()
+			   : Hash == Level3Hash ? Level3.GetOuterBuffer()
 			   : Hash == Level4Hash ? Level4
 									: SharedBuffer();
 	};
@@ -907,8 +1002,9 @@ TEST_CASE("usonpackage.serialization")
 		const CbAttachment* const Level4Attachment = Package.FindAttachment(Level4Hash);
 		CHECK((Level2Attachment && Level2Attachment->AsObject().Equals(Level2)));
 		CHECK((Level3Attachment && Level3Attachment->AsObject().Equals(Level3)));
-		CHECK((Level4Attachment && Level4Attachment->AsBinary() != Level4 &&
-			   Level4Attachment->AsBinary().GetView().EqualBytes(Level4.GetView())));
+		REQUIRE(Level4Attachment);
+		CHECK(Level4Attachment->AsBinary() != Level4);
+		CHECK(Level4Attachment->AsBinary().GetView().EqualBytes(Level4.GetView()));
 
 		CHECK(std::is_sorted(begin(Package.GetAttachments()), end(Package.GetAttachments())));
 
@@ -932,15 +1028,15 @@ TEST_CASE("usonpackage.serialization")
 
 	// Out of Order
 	{
-		CbWriter Writer;
-		Writer.AddBinary(Level2.GetBuffer());
-		Writer.AddObjectAttachment(Level2Hash);
-		Writer.AddBinary(Level4);
-		Writer.AddBinaryAttachment(Level4Hash);
+		CbWriter	 Writer;
+		CbAttachment Attachment2(Level2, Level2Hash);
+		Attachment2.Save(Writer);
+		CbAttachment Attachment4(Level4);
+		Attachment4.Save(Writer);
+		Writer.AddHash(Level1Hash);
 		Writer.AddObject(Level1);
-		Writer.AddObjectAttachment(Level1Hash);
-		Writer.AddBinary(Level3.GetBuffer());
-		Writer.AddObjectAttachment(Level3Hash);
+		CbAttachment Attachment3(Level3, Level3Hash);
+		Attachment3.Save(Writer);
 		Writer.AddNull();
 
 		CbFieldIterator Fields = Writer.Save();
@@ -961,11 +1057,9 @@ TEST_CASE("usonpackage.serialization")
 		const MemoryView FieldsOuterBufferView = Fields.GetOuterBuffer().GetView();
 
 		CHECK(Level2Attachment->AsObject().Equals(Level2));
-		CHECK(FieldsOuterBufferView.Contains(Level2Attachment->AsBinary().GetView()));
 		CHECK(Level2Attachment->GetHash() == Level2Hash);
 
 		CHECK(Level3Attachment->AsObject().Equals(Level3));
-		CHECK(FieldsOuterBufferView.Contains(Level3Attachment->AsBinary().GetView()));
 		CHECK(Level3Attachment->GetHash() == Level3Hash);
 
 		CHECK(Level4Attachment->AsBinary().GetView().EqualBytes(Level4.GetView()));
@@ -983,21 +1077,23 @@ TEST_CASE("usonpackage.serialization")
 		Writer.Reset();
 		FromArchive.Save(Writer);
 		CbFieldIterator Saved = Writer.Save();
-		CHECK(Saved.AsObject().Equals(Level1));
+
+		CHECK(Saved.AsHash() == Level1Hash);
 		++Saved;
-		CHECK(Saved.AsObjectAttachment() == Level1Hash);
+		CHECK(Saved.AsObject().Equals(Level1));
 		++Saved;
-		CHECK(Saved.AsBinaryView().EqualBytes(Level2.GetView()));
+		CHECK_EQ(Saved.AsObjectAttachment(), Level2Hash);
 		++Saved;
-		CHECK(Saved.AsObjectAttachment() == Level2Hash);
+		CHECK(Saved.AsObject().Equals(Level2));
 		++Saved;
-		CHECK(Saved.AsBinaryView().EqualBytes(Level3.GetView()));
+		CHECK_EQ(Saved.AsObjectAttachment(), Level3Hash);
 		++Saved;
-		CHECK(Saved.AsObjectAttachment() == Level3Hash);
+		CHECK(Saved.AsObject().Equals(Level3));
 		++Saved;
-		CHECK(Saved.AsBinaryView().EqualBytes(Level4.GetView()));
+		CHECK_EQ(Saved.AsBinaryAttachment(), Level4Hash);
 		++Saved;
-		CHECK(Saved.AsBinaryAttachment() == Level4Hash);
+		SharedBuffer SavedLevel4Buffer = SharedBuffer::MakeView(Saved.AsBinaryView());
+		CHECK(SavedLevel4Buffer.GetView().EqualBytes(Level4.GetView()));
 		++Saved;
 		CHECK(Saved.IsNull());
 		++Saved;