Fix OneToManyRelations and VectorMembers in cloned objects (#583)

* Fix issue with relations after reading and add test

Fix an issue where it's not possible to push_back to a cloned object that has
been read because all the OneToManyRelations in the same collection
point to the same vector. The fix is to actually make a copy of the
part of the vector that belongs to that object.

* Fix also vector members

* Implement the fix in clone()

* Add a test for a collection with empty relations

* Add test with rntuple

* Fix a leak in the implementation

* Add test case for SIO

* Add test reading cloned objects

---------

Co-authored-by: jmcarcell <[email protected]>

python/templates/Object.cc.jinja2

@@ -16,7 +16,7 @@
 
 {{ utils.namespace_open(class.namespace) }}
 
-{{ macros.constructors_destructors(class.bare_type, Members) }}
+{{ macros.constructors_destructors(class.bare_type, Members, OneToManyRelations + VectorMembers) }}
 
 {{ class.bare_type }}::{{ class.bare_type }}(const Mutable{{ class.bare_type }}& other): {{ class.bare_type }}(other.m_obj) {}
 

python/templates/macros/implementations.jinja2

@@ -1,4 +1,4 @@
-{% macro constructors_destructors(type, members, prefix='') %}
+{% macro constructors_destructors(type, members, multi_relations=[], prefix='') %}
 {% set full_type = prefix + type %}
 
 {{ full_type }}::{{ full_type }}() :
@@ -19,7 +19,23 @@
 }
 
 Mutable{{ type }} {{ full_type }}::clone() const {
+{% if prefix %}
   return Mutable{{ type }}(podio::utils::MaybeSharedPtr(new {{ type }}Obj(*m_obj), podio::utils::MarkOwned));
+{% else %}
+  auto tmp = new {{ type }}Obj(podio::ObjectID{}, m_obj->data);
+{% for relation in multi_relations %}
+  tmp->m_{{ relation.name }} = new std::vector<{{ relation.full_type }}>();
+  // If the current object has been read from a file, then the object may only have a slice of the relation vector
+  // so this slice has to be copied in case we want to modify it
+  tmp->m_{{ relation.name }}->reserve(m_obj->m_{{ relation.name }}->size());
+  for (size_t i = m_obj->data.{{ relation.name }}_begin; i < m_obj->data.{{ relation.name }}_end; i++) {
+    tmp->m_{{ relation.name }}->emplace_back((*m_obj->m_{{ relation.name }})[i]);
+  }
+  tmp->data.{{ relation.name }}_begin = 0;
+  tmp->data.{{ relation.name }}_end = tmp->m_{{ relation.name }}->size();
+{% endfor %}
+  return Mutable{{ type }}(podio::utils::MaybeSharedPtr(tmp, podio::utils::MarkOwned));
+{% endif %}
 }
 
 {{ full_type }}::{{ full_type }}(podio::utils::MaybeSharedPtr<{{ type }}Obj> obj) : m_obj(std::move(obj)) {}

tests/unittests/unittest.cpp

@@ -1,5 +1,6 @@
 // STL
 #include <cstdint>
+#include <filesystem>
 #include <map>
 #include <sstream>
 #include <stdexcept>
@@ -25,9 +26,11 @@
 #if PODIO_ENABLE_SIO
   #include "podio/SIOLegacyReader.h"
   #include "podio/SIOReader.h"
+  #include "podio/SIOWriter.h"
 #endif
 
 #if PODIO_ENABLE_RNTUPLE
+  #include "podio/RNTupleReader.h"
   #include "podio/RNTupleWriter.h"
 #endif
 
@@ -1234,6 +1237,97 @@ void runCheckConsistencyTest(const std::string& filename) {
   REQUIRE_THAT(superfluous, UnorderedEquals<std::string>({"non-existant"}));
 }
 
+template <typename ReaderT, typename WriterT>
+void runRelationAfterCloneCheck(const std::string& filename = "unittest_relations_after_cloning.root") {
+  auto [hitColl, clusterColl, vecMemColl, userDataColl] = createCollections();
+  auto frame = podio::Frame();
+  frame.put(std::move(hitColl), "hits");
+  frame.put(std::move(clusterColl), "clusters");
+  frame.put(std::move(vecMemColl), "vectors");
+  // Empty relations
+  auto emptyColl = ExampleClusterCollection();
+  emptyColl.create();
+  emptyColl.create();
+  frame.put(std::move(emptyColl), "emptyClusters");
+  auto writer = WriterT(filename);
+  writer.writeFrame(frame, podio::Category::Event);
+  writer.finish();
+  auto reader = ReaderT();
+  reader.openFile(filename);
+  auto readFrame = podio::Frame(reader.readNextEntry(podio::Category::Event));
+
+  auto& clusters = readFrame.get<ExampleClusterCollection>("clusters");
+
+  auto nCluster = clusters[0].clone();
+  REQUIRE(nCluster.Hits().size() == 1);
+
+  auto hit = MutableExampleHit(420, {}, {}, {}, {});
+  nCluster.addHits(hit);
+  REQUIRE(nCluster.Hits().size() == 2);
+  REQUIRE(nCluster.Hits()[1].cellID() == 420);
+
+  auto nCluster2 = nCluster.clone();
+  REQUIRE(nCluster2.Hits().size() == 2);
+  auto anotherHit = MutableExampleHit(421, {}, {}, {}, {});
+  nCluster2.addHits(anotherHit);
+  REQUIRE(nCluster2.Hits().size() == 3);
+  REQUIRE(nCluster2.Hits()[2].cellID() == 421);
+
+  auto& vectors = readFrame.get<ExampleWithVectorMemberCollection>("vectors");
+  auto nVec = vectors[0].clone();
+  REQUIRE(nVec.count().size() == 2);
+  nVec.addcount(420);
+  REQUIRE(nVec.count().size() == 3);
+  REQUIRE(nVec.count()[2] == 420);
+
+  auto newClusterCollection = ExampleClusterCollection();
+  auto newHitCollection = ExampleHitCollection();
+  auto& emptyClusters = readFrame.get<ExampleClusterCollection>("emptyClusters");
+  auto nEmptyCluster = emptyClusters[0].clone();
+  REQUIRE(nEmptyCluster.Hits().empty());
+  nEmptyCluster.addHits(hit);
+  REQUIRE(nEmptyCluster.Hits().size() == 1);
+  REQUIRE(nEmptyCluster.Hits()[0].cellID() == 420);
+  nEmptyCluster.addHits(anotherHit);
+  REQUIRE(nEmptyCluster.Hits().size() == 2);
+  REQUIRE(nEmptyCluster.Hits()[1].cellID() == 421);
+  newClusterCollection.push_back(nEmptyCluster);
+  newHitCollection.push_back(hit);
+  newHitCollection.push_back(anotherHit);
+
+  // Test cloned objects after writing and reading
+  auto newName = std::filesystem::path(filename)
+                     .replace_extension("_cloned" + std::filesystem::path(filename).extension().string())
+                     .string();
+  auto newWriter = WriterT(newName);
+  auto newFrame = podio::Frame();
+  newFrame.put(std::move(newClusterCollection), "emptyClusters");
+  newFrame.put(std::move(newHitCollection), "hits");
+  newWriter.writeFrame(newFrame, podio::Category::Event);
+  newWriter.finish();
+  auto newReader = ReaderT();
+  newReader.openFile(newName);
+  auto afterCloneFrame = podio::Frame(newReader.readNextEntry(podio::Category::Event));
+
+  auto& newEmptyClusters = afterCloneFrame.get<ExampleClusterCollection>("emptyClusters");
+  auto oneHitCluster = newEmptyClusters[0].clone();
+  auto newHit = ExampleHit(422, 0., 0., 0., 0.);
+  auto newAnotherHit = ExampleHit(423, 0., 0., 0., 0.);
+  REQUIRE(nEmptyCluster.Hits().size() == 2);
+  REQUIRE(nEmptyCluster.Hits()[0].cellID() == 420);
+  REQUIRE(nEmptyCluster.Hits()[1].cellID() == 421);
+  nEmptyCluster.addHits(newHit);
+  REQUIRE(nEmptyCluster.Hits().size() == 3);
+  REQUIRE(nEmptyCluster.Hits()[2].cellID() == 422);
+  nEmptyCluster.addHits(newAnotherHit);
+  REQUIRE(nEmptyCluster.Hits().size() == 4);
+  REQUIRE(nEmptyCluster.Hits()[3].cellID() == 423);
+}
+
+TEST_CASE("Relations after cloning with TTrees", "[ASAN-FAIL][UBSAN-FAIL][relations][basics]") {
+  runRelationAfterCloneCheck<podio::ROOTReader, podio::ROOTWriter>("unittests_relations_after_cloning.root");
+}
+
 TEST_CASE("ROOTWriter consistent frame contents", "[ASAN-FAIL][UBSAN-FAIL][THREAD-FAIL][basics][root]") {
   // The UBSAN-FAIL and TSAN-FAIL only happens on clang12 in CI.
   runConsistentFrameTest<podio::ROOTWriter>("unittests_frame_consistency.root");
@@ -1244,6 +1338,12 @@ TEST_CASE("ROOTWriter check consistency", "[ASAN-FAIL][UBSAN-FAIL][basics][root]
 }
 
 #if PODIO_ENABLE_RNTUPLE
+
+TEST_CASE("Relations after cloning with RNTuple", "[relations][basics]") {
+  runRelationAfterCloneCheck<podio::RNTupleReader, podio::RNTupleWriter>(
+      "unittests_relations_after_cloning_rntuple.root");
+}
+
 TEST_CASE("RNTupleWriter consistent frame contents", "[basics][root]") {
   runConsistentFrameTest<podio::RNTupleWriter>("unittests_frame_consistency_rntuple.root");
 }
@@ -1253,3 +1353,11 @@ TEST_CASE("RNTupleWriter check consistency", "[basics][root]") {
 }
 
 #endif
+
+#if PODIO_ENABLE_SIO
+
+TEST_CASE("Relations after cloning with SIO", "[relations][basics]") {
+  runRelationAfterCloneCheck<podio::SIOReader, podio::SIOWriter>("unittests_relations_after_cloning.sio");
+}
+
+#endif