Jbl/1020721 preserve order in attachmentsdirectory

CMakeLists.txt

@@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.15)
 cmake_policy(SET CMP0091 NEW) # enable new "MSVC runtime library selection" (https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html)
 
 project(libCZI 
-      VERSION 0.53.0
+      VERSION 0.53.1
       HOMEPAGE_URL "https://github.com/ZEISS/libczi"
       DESCRIPTION "libCZI is an Open Source Cross-Platform C++ library to read and write CZI")
 

Src/libCZI/CziAttachmentsDirectory.cpp

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: LGPL-3.0-or-later
 
 #include "CziAttachmentsDirectory.h"
+#include <algorithm>
 #include <cstring>
 
 /*static*/bool CCziAttachmentsDirectoryBase::CompareForEquality_Id(const AttachmentEntry& a, const AttachmentEntry& b)
@@ -62,14 +63,22 @@ bool CCziAttachmentsDirectory::TryGetAttachment(int index, AttachmentEntry& entr
 
 bool CWriterCziAttachmentsDirectory::TryAddAttachment(const AttachmentEntry& entry)
 {
-    auto insert = this->attachments.insert(entry);
-    return insert.second;
+    if (std::find_if(
+        this->attachments_.cbegin(),
+        this->attachments_.cend(),
+        [&entry](const AttachmentEntry& x)->bool { return CCziAttachmentsDirectoryBase::CompareForEquality_Id(x, entry); }) != this->attachments_.cend())
+    {
+        return false;
+    }
+
+    this->attachments_.push_back(entry);
+    return true;
 }
 
 bool CWriterCziAttachmentsDirectory::EnumEntries(const std::function<bool(int index, const AttachmentEntry&)>& func) const
 {
     int index = 0;
-    for (auto it = this->attachments.cbegin(); it != this->attachments.cend(); ++it)
+    for (auto it = this->attachments_.cbegin(); it != this->attachments_.cend(); ++it)
     {
         if (!func(index++, *it))
         {
@@ -82,39 +91,7 @@ bool CWriterCziAttachmentsDirectory::EnumEntries(const std::function<bool(int in
 
 int CWriterCziAttachmentsDirectory::GetAttachmentCount() const
 {
-    return (int)this->attachments.size();
-}
-
-bool CWriterCziAttachmentsDirectory::AttachmentEntriesCompare::operator()(const AttachmentEntry& a, const AttachmentEntry& b) const
-{
-    // we shall return true if a is considered to go before b in the strict weak ordering the function defines
-    int r = a.ContentGuid.compare(b.ContentGuid);
-    if (r < 0)
-    {
-        return true;
-    }
-    else if (r > 0)
-    {
-        return false;
-    }
-
-    r = memcmp(&a.ContentFileType, &b.ContentFileType, sizeof(a.ContentFileType));
-    if (r < 0)
-    {
-        return true;
-    }
-    else if (r > 0)
-    {
-        return false;
-    }
-
-    r = memcmp(&a.Name, &b.Name, sizeof(a.Name));
-    if (r < 0)
-    {
-        return true;
-    }
-
-    return false;
+    return (int)this->attachments_.size();
 }
 
 //-----------------------------------------------------------------------------
@@ -199,12 +176,12 @@ bool CReaderWriterCziAttachmentsDirectory::TryRemoveAttachment(int key, Attachme
 
 bool CReaderWriterCziAttachmentsDirectory::TryAddAttachment(const AttachmentEntry& entry, int* key)
 {
-    for (auto it = this->attchmnts.cbegin(); it != this->attchmnts.cend(); ++it)
+    if (std::find_if(
+        this->attchmnts.cbegin(),
+        this->attchmnts.cend(),
+        [&entry](const std::pair<int, AttachmentEntry>& x)->bool { return CCziAttachmentsDirectoryBase::CompareForEquality_Id(x.second, entry); }) != this->attchmnts.cend())
     {
-        if (CCziAttachmentsDirectoryBase::CompareForEquality_Id(it->second, entry))
-        {
-            return false;
-        }
+        return false;
     }
 
     this->AddAttachment(entry, key);

Src/libCZI/CziAttachmentsDirectory.h

@@ -25,6 +25,13 @@ class CCziAttachmentsDirectoryBase
         std::int64_t allocatedSize;
     };
 
+    /// Compare the properties which make up the identity of an AttachmentEntry,
+    /// i.e. the GUID, the name and the content file type, for equality.
+    ///
+    /// \param  a   The first AttachmentEntry to process.
+    /// \param  b   The second AttachmentEntry to process.
+    ///
+    /// \returns    True if the identifier-properties compare as equal; false otherwise.
     static bool CompareForEquality_Id(const AttachmentEntry& a, const AttachmentEntry& b);
 };
 
@@ -51,11 +58,7 @@ class CWriterCziAttachmentsDirectory : public CCziAttachmentsDirectoryBase
     int GetAttachmentCount() const;
     bool EnumEntries(const std::function<bool(int index, const AttachmentEntry&)>& func) const;
 private:
-    struct AttachmentEntriesCompare {
-        bool operator() (const AttachmentEntry& a, const AttachmentEntry& b) const;
-    };
-
-    std::set<AttachmentEntry, AttachmentEntriesCompare> attachments;
+    std::vector<AttachmentEntry> attachments_;
 };
 
 class CReaderWriterCziAttachmentsDirectory : public CCziAttachmentsDirectoryBase

Src/libCZI/CziWriter.cpp

@@ -1160,7 +1160,7 @@ void CCziWriter::WriteAttachmentDirectory()
     }
 }
 
-std::tuple<std::uint64_t, std::uint64_t>  CCziWriter::WriteCurrentAttachmentsDirectory()
+std::tuple<std::uint64_t, std::uint64_t> CCziWriter::WriteCurrentAttachmentsDirectory()
 {
     CWriterUtils::AttachmentDirWriteInfo info;
     if (this->attachmentDirectorySegment.IsValid())

Src/libCZI_UnitTests/test_readerwriter.cpp

@@ -1352,3 +1352,38 @@ INSTANTIATE_TEST_SUITE_P(
         SubBlockPyramidType::None,
         SubBlockPyramidType::SingleSubBlock,
         SubBlockPyramidType::MultiSubBlock));
+
+TEST(CziReaderWriter, TryAddingDuplicateAttachmentToCziReaderWriterAndExpectError)
+{
+    const auto in_out_stream = make_shared<CMemInputOutputStream>(0);
+    const auto reader_writer = CreateCZIReaderWriter();
+    reader_writer->Create(in_out_stream);
+
+    constexpr uint8_t data[] = { 1,2,3 };
+
+    AddAttachmentInfo add_attachment_info;
+    add_attachment_info.contentGuid = GUID{ 1, 2, 3, {4, 5, 6, 7, 8, 9, 10, 11} };
+    add_attachment_info.SetContentFileType("ABC");
+    add_attachment_info.SetName("Test");
+    add_attachment_info.ptrData = data;
+    add_attachment_info.dataSize = sizeof(data);
+    reader_writer->SyncAddAttachment(add_attachment_info);
+
+    // now, try to add it a second time
+    EXPECT_THROW(reader_writer->SyncAddAttachment(add_attachment_info), LibCZIException);
+
+    // a duplicate attachment means - same Guid, same name, same content-type,
+    //  so now let's try to add it with a different name (which should work)
+    add_attachment_info.SetName("Test2");
+    reader_writer->SyncAddAttachment(add_attachment_info);
+
+    // now with a different content-type (which should also work)
+    add_attachment_info.SetName("Test");
+    add_attachment_info.SetContentFileType("ABCD");
+    reader_writer->SyncAddAttachment(add_attachment_info);
+
+    // and finally with a different GUID
+    add_attachment_info.SetContentFileType("ABC");
+    add_attachment_info.contentGuid = GUID{ 111, 2, 3, {4, 5, 6, 7, 8, 9, 10, 11} };
+    reader_writer->SyncAddAttachment(add_attachment_info);
+}

Src/libCZI_UnitTests/test_writer.cpp

@@ -2121,3 +2121,26 @@ TEST(CziWriter, WriteReadCompressedZStd1ImageBrg48LowPacking)
     constexpr PixelType pixelType = PixelType::Bgr48;
     _testWriteReadCompressedImageZStd1LowPacking(61, 61, pixelType, 2, true);
 }
+
+TEST(CziWriter, TryAddingDuplicateAttachmentToCziWriterAndExpectError)
+{
+    const auto writer = CreateCZIWriter();
+    const auto output_stream = make_shared<CMemOutputStream>(0);
+
+    const auto czi_writer_info = std::make_shared<libCZI::CCziWriterInfo>(GUID{ 0, 0, 0, {0, 0, 0, 0, 0, 0, 0, 0} });
+
+    writer->Create(output_stream, czi_writer_info);
+
+    constexpr uint8_t data[] = { 1,2,3 };
+
+    AddAttachmentInfo add_attachment_info;
+    add_attachment_info.contentGuid = GUID{ 1, 2, 3, {4, 5, 6, 7, 8, 9, 10, 11} };
+    add_attachment_info.SetContentFileType("ABC");
+    add_attachment_info.SetName("Test");
+    add_attachment_info.ptrData = data;
+    add_attachment_info.dataSize = sizeof(data);
+    writer->SyncAddAttachment(add_attachment_info);
+
+    // now, try to add it a second time
+    EXPECT_THROW(writer->SyncAddAttachment(add_attachment_info), LibCZIException);
+}