Allow types from different data models in interfaces (#714)

* add detail function for object rank that can be used for comparison

add detecting if `uintptr_t` is defined

* add extension data model with interface

add test for interface with types from data model and its extension

* hide rank value in a proxy

* rename 'Rank' to 'OrderKey'

* add comments, mention in docs

* compare with `std::less`, remove obsolete typedefs, sprinkle `noexcept`

* allow cross-model interfaces in relations

* test relations IO with cross-edm interface

* test links IO with cross-edm interfaces

* fix missing link to extension datamodel in cmake testing function

.gitignore

@@ -5,6 +5,7 @@ install
 tests/src
 tests/datamodel
 tests/extension_model
+tests/interface_extension_model
 tests/datamodeljulia
 tests/unittests/Project.toml
 tests/unittests/Manifest.toml

cmake/podioTest.cmake

@@ -27,6 +27,6 @@ function(CREATE_PODIO_TEST sourcefile additional_libs)
   add_executable( ${name} ${sourcefile} )
   add_test(NAME ${name} COMMAND ${name})
 
-  target_link_libraries(${name} PRIVATE TestDataModel ExtensionDataModel ${additional_libs})
+  target_link_libraries(${name} PRIVATE TestDataModel ExtensionDataModel InterfaceExtensionDataModel ${additional_libs})
   PODIO_SET_TEST_ENV(${name})
 endfunction()

doc/datamodel_syntax.md

@@ -268,5 +268,6 @@ The podio `PODIO_GENERATE_DATAMODEL` cmake macro has gained an additional parame
 ### Potential pitfalls
 - The cmake macros do not handle linking against an upstream datamodel automatically. It is the users responsibility to explicitly link against the upstream datamodel.
 - When generating two datamodels side-by-side and into the same output directory and using the `ROOT` backend, the generated `selection.xml` file might be overwritten if both datamodels are generated into the same output directory.
+- The [interfaces](#definition-of-custom-interfaces) defined in a datamodel can list the datatypes defined in an upstream datamodel only if both datamodels have the same [`getSyntax`](#global-options) value.
 
 Limiting this functionality to one upstream datamodel is a conscious choice to limit the potential for abuse of this feature.

include/podio/detail/OrderKey.h

@@ -0,0 +1,28 @@
+#ifndef PODIO_DETAIL_ORDERKEY_H
+#define PODIO_DETAIL_ORDERKEY_H
+#include <functional>
+namespace podio::detail {
+/// Internal class allowing datatype objects to be placed in data structures like maps and sets by providing a way to
+/// compare them. The comparison is based on addresses of their internal data objects.
+///
+/// This class is intended to be used as the return value of internal `podio::detail::getOrderKey` free functions. These
+/// functions are friends of each datatype, allowing them to access the internal data objects and define less-than
+/// comparison operators for both datatypes and interface types.
+///
+/// The friend free function design is used in order to reduce the coupling between interfaces and datatypes. Interfaces
+/// do not need to be friends of datatypes to define the less-than comparison operator, which allows using datatypes
+/// from different datamodels in an interface type.
+class OrderKey {
+public:
+  OrderKey(void* orderKey) noexcept : m_orderKey(orderKey) {
+  }
+  friend bool operator<(const OrderKey& lhs, const OrderKey& rhs) noexcept {
+    return std::less<void*>{}(lhs.m_orderKey, rhs.m_orderKey);
+  }
+
+private:
+  void* m_orderKey;
+};
+} // namespace podio::detail
+
+#endif // PODIO_DETAIL_ORDERKEY_H

python/podio/test_Frame.py

@@ -47,6 +47,13 @@
     "interface_examples",
 }
 
+# The expected collections from the interface extension (only present in the other_events category)
+EXPECTED_INTERFACE_EXTENSION_COLL_NAMES = {
+    "anotherHits",
+    "extension_interface_relation",
+    "extension_interface_links",
+}
+
 # The expected parameter names in each frame
 EXPECTED_PARAM_NAMES = {
     "anInt",
@@ -167,7 +174,9 @@ def test_frame_collections(self):
         self.assertEqual(set(self.event.getAvailableCollections()), EXPECTED_COLL_NAMES)
         self.assertEqual(
             set(self.other_event.getAvailableCollections()),
-            EXPECTED_COLL_NAMES.union(EXPECTED_EXTENSION_COLL_NAMES),
+            EXPECTED_COLL_NAMES.union(EXPECTED_EXTENSION_COLL_NAMES).union(
+                EXPECTED_INTERFACE_EXTENSION_COLL_NAMES
+            ),
         )
 
         # Not going over all collections here, as that should all be covered by the

python/podio/test_Reader.py

@@ -75,9 +75,9 @@ def test_frame_iterator_invalid_category(self):
     def test_available_datamodels(self):
         """Make sure that the datamodel information can be retrieved"""
         datamodels = self.reader.datamodel_definitions
-        self.assertEqual(len(datamodels), 2)
+        self.assertEqual(len(datamodels), 3)
         for model in datamodels:
-            self.assertTrue(model in ("datamodel", "extension_model"))
+            self.assertTrue(model in ("datamodel", "extension_model", "interface_extension_model"))
 
         self.assertEqual(self.reader.current_file_version("datamodel"), build_version)
 

python/podio_gen/cpp_generator.py

@@ -340,9 +340,10 @@ def _preprocess_for_collection(self, datatype):
                         self._build_include_for_class(relation.bare_type, include_from)
                     )
                     for int_type in relation.interface_types:
+                        int_type_include_from = self._needs_include(int_type.full_type)
                         includes_cc.add(
                             self._build_include_for_class(
-                                int_type.bare_type + "Collection", include_from
+                                int_type.bare_type + "Collection", int_type_include_from
                             )
                         )
                 else:

python/templates/Interface.h.jinja2

@@ -12,6 +12,7 @@
 
 #include "podio/ObjectID.h"
 #include "podio/utilities/TypeHelpers.h"
+#include "podio/detail/OrderKey.h"
 
 #include <memory>
 #include <ostream>
@@ -59,7 +60,7 @@ private:
 {{ macros.member_getters_concept(Members, use_get_syntax) }}
     virtual const std::type_info& typeInfo() const = 0;
     virtual bool equal(const Concept* rhs) const = 0;
-    virtual const void* objAddress() const = 0;
+    virtual podio::detail::OrderKey objOrderKey() const = 0;
   };
 
   template<typename ValueT>
@@ -88,8 +89,8 @@ private:
       return false;
     }
 
-    const void* objAddress() const final {
-      return m_value.m_obj.get();
+    podio::detail::OrderKey objOrderKey() const final {
+      return podio::detail::getOrderKey(m_value);
     }
 
 {{ macros.member_getters_model(Members, use_get_syntax) }}
@@ -169,7 +170,7 @@ public:
   }
 
   friend bool operator<(const {{ class.bare_type }}& lhs, const {{ class.bare_type }}& rhs) {
-    return lhs.m_self->objAddress() < rhs.m_self->objAddress();
+    return lhs.m_self->objOrderKey() < rhs.m_self->objOrderKey();
   }
 
 {{ macros.member_getters(Members, use_get_syntax) }}

python/templates/Object.cc.jinja2

@@ -43,3 +43,7 @@
                       VectorMembers, use_get_syntax)}}
 
 {{ utils.namespace_close(class.namespace) }}
+
+podio::detail::OrderKey podio::detail::getOrderKey(const {{ class.namespace }}::{{ class.bare_type }}& obj) {
+  return podio::detail::OrderKey{obj.m_obj.get()};
+}

python/templates/Object.h.jinja2

@@ -12,6 +12,7 @@
 {% endfor %}
 
 #include "podio/utilities/MaybeSharedPtr.h"
+#include "podio/detail/OrderKey.h"
 
 #include <ostream>
 #include <cstdint>
@@ -22,6 +23,11 @@
 
 {{ utils.forward_decls(forward_declarations) }}
 
+namespace podio::detail {
+// Internal function used in less comparison operators of the datatypes and interface types
+OrderKey getOrderKey(const {{ class.namespace }}::{{ class.bare_type }}& obj);
+};
+
 {{ utils.namespace_open(class.namespace) }}
 class Mutable{{ class.bare_type }};
 class {{ class.bare_type }}Collection;
@@ -34,9 +40,7 @@ class {{ class.bare_type }} {
   friend class {{ class.bare_type }}Collection;
   friend class {{ class.full_type }}CollectionData;
   friend class {{ class.bare_type }}CollectionIterator;
-{% for interface in using_interface_types %}
-  friend class {{ interface }};
-{% endfor %}
+  friend podio::detail::OrderKey podio::detail::getOrderKey(const {{ class.bare_type }} & obj);
 
 public:
   using mutable_type = Mutable{{ class.bare_type }};

python/templates/macros/declarations.jinja2

@@ -97,7 +97,7 @@
   bool operator!=(const {{ inverse_type }}& other) const { return !(*this == other); }
 
   // less comparison operator, so that objects can be e.g. stored in sets.
-  bool operator<(const {{ full_type }}& other) const { return m_obj < other.m_obj; }
+  bool operator<(const {{ full_type }}& other) const { return podio::detail::getOrderKey(*this) < podio::detail::getOrderKey(other); }
 
   podio::ObjectID id() const { return getObjectID(); }
 

tests/CMakeLists.txt

@@ -44,6 +44,21 @@ PODIO_ADD_ROOT_IO_DICT(ExtensionDataModelDict ExtensionDataModel "${ext_headers}
 
 PODIO_ADD_SIO_IO_BLOCKS(ExtensionDataModel "${ext_headers}" "${ext_sources}")
 
+# Build the interface extension data model and link it against the upstream model
+PODIO_GENERATE_DATAMODEL(interface_extension_model datalayout_interface_extension.yaml iext_headers iext_sources
+  UPSTREAM_EDM datamodel:datalayout.yaml
+  IO_BACKEND_HANDLERS ${PODIO_IO_HANDLERS}
+  OUTPUT_FOLDER ${CMAKE_CURRENT_SOURCE_DIR}/interface_extension_model)
+
+PODIO_ADD_DATAMODEL_CORE_LIB(InterfaceExtensionDataModel "${iext_headers}" "${iext_sources}"
+  OUTPUT_FOLDER ${CMAKE_CURRENT_SOURCE_DIR}/interface_extension_model)
+target_link_libraries(InterfaceExtensionDataModel PUBLIC TestDataModel)
+
+PODIO_ADD_ROOT_IO_DICT(InterfaceExtensionDataModelDict InterfaceExtensionDataModel "${iext_headers}" ${CMAKE_CURRENT_SOURCE_DIR}/interface_extension_model/src/selection.xml
+  OUTPUT_FOLDER ${CMAKE_CURRENT_SOURCE_DIR}/interface_extension_model)
+
+PODIO_ADD_SIO_IO_BLOCKS(InterfaceExtensionDataModel "${iext_headers}" "${iext_sources}")
+
 # Add a legacy test case based on a base executable and a version for which an
 # input file exists
 macro(ADD_PODIO_LEGACY_TEST version base_test input_file)

tests/datalayout_interface_extension.yaml

@@ -0,0 +1,49 @@
+---
+schema_version: 1
+options:
+  getSyntax: False
+  exposePODMembers: False
+  includeSubfolder: True
+
+components:
+  iextension::PolarVector:
+    Members:
+      - float r
+      - float theta
+      - float phi
+
+datatypes:
+  iextension::AnotherHit:
+    Author: "Mateusz Jakub Fila"
+    Description: "A datatype in the extension with components from the extension and an upstream datamodel"
+    Members:
+     - unsigned long long cellID       // cellID
+     - SimpleStruct aStruct            // component defined in an upstream datamodel
+     - iextension::PolarVector aVector // component defined in the extension
+     - double energy [GeV]             // measured energy deposit
+
+  iextension::ExampleWithInterfaceRelation:
+    Description: "Datatype that uses an interface type as one of its relations"
+    Author: "Mateusz Jakub Fila"
+    OneToOneRelations:
+      - iextension::EnergyInterface singleEnergy // single relation
+    OneToManyRelations:
+      - iextension::EnergyInterface manyEnergies // multiple relations
+
+interfaces:
+  iextension::EnergyInterface:
+    Description: "Generic interface for types with an energy member"
+    Author: "Mateusz Jakub Fila"
+    Types:
+      - ExampleHit
+      - ExampleMC
+      - ExampleCluster
+      - iextension::AnotherHit
+    Members:
+      - double energy // the energy
+links:
+  iextension::TestInterfaceLink:
+    Description: "A link with an interface type for testing"
+    Author: "Mateusz Jakub Fila"
+    From: ExampleHit
+    To: iextension::EnergyInterface

tests/read_frame.h

@@ -9,6 +9,11 @@
 #include "extension_model/ExternalComponentTypeCollection.h"
 #include "extension_model/ExternalRelationTypeCollection.h"
 
+#include "interface_extension_model/AnotherHitCollection.h"
+#include "interface_extension_model/EnergyInterface.h"
+#include "interface_extension_model/ExampleWithInterfaceRelationCollection.h"
+#include "interface_extension_model/TestInterfaceLinkCollection.h"
+
 #include "podio/Frame.h"
 
 #include <iostream>
@@ -97,6 +102,45 @@ void checkInterfaceCollection(const podio::Frame& event) {
   ASSERT(iface1Rels[2] == clusters[0], "OneToManyRelations to interface not persisted correctly")
 }
 
+void checkInterfaceExtension(const podio::Frame& event) {
+  const auto& interfaceColl =
+      event.get<iextension::ExampleWithInterfaceRelationCollection>("extension_interface_relation");
+  ASSERT(interfaceColl.size() == 2, "extension_inteface_relation should have two elements")
+
+  const auto& hits = event.get<ExampleHitCollection>("hits");
+  const auto& anotherHits = event.get<iextension::AnotherHitCollection>("anotherHits");
+
+  const auto iface0 = interfaceColl[0];
+  const auto iface1 = interfaceColl[1];
+
+  ASSERT(iface0.singleEnergy() == hits[0], "OneToOneRelation singleEnergy not persisted as expected")
+  ASSERT(iface1.singleEnergy() == anotherHits[0], "OneToOneRelation singleEnergy not persisted as expected")
+
+  const auto iface0Rels = iface0.manyEnergies();
+  ASSERT(iface0Rels.size() == 2, "OneToManyRelation to interface does not have the expected number of related elements")
+  ASSERT(iface0Rels[0] == hits[0], "OneToManyRelations to interface not persisted correctly")
+  ASSERT(iface0Rels[1] == anotherHits[0], "OneToManyRelations to interface not persisted correctly")
+
+  const auto iface1Rels = iface1.manyEnergies();
+  ASSERT(iface1Rels.size() == 2, "OneToManyRelation to interface does not have the expected number of related elements")
+  ASSERT(iface1Rels[0] == hits[0], "OneToManyRelations to interface not persisted correctly")
+  ASSERT(iface1Rels[1] == anotherHits[0], "OneToManyRelations to interface not persisted correctly")
+
+  const auto& linkColl = event.get<iextension::TestInterfaceLinkCollection>("extension_interface_links");
+  ASSERT(linkColl.size() == 3, "extension_interface_links should have three elements")
+  auto link = linkColl[0];
+  ASSERT(link.get<ExampleHit>() == hits[0], "Link to interface 'FROM' not persisted correctly");
+  ASSERT(link.get<iextension::EnergyInterface>() == anotherHits[0], "Link to interface 'TO' not persisted correctly");
+  ASSERT(link.getWeight() == 1.23f, "Link to interface weight not persisted correctly");
+  link = linkColl[1];
+  ASSERT(link.get<ExampleHit>() == hits[1], "Link to interface 'FROM' not persisted correctly");
+  ASSERT(link.get<iextension::EnergyInterface>() == anotherHits[1], "Link to interface 'TO' not persisted correctly");
+  ASSERT(link.getWeight() == 3.14f, "Link to interface weight not persisted correctly");
+  link = linkColl[2];
+  ASSERT(link.get<ExampleHit>() == hits[0], "Link to interface 'FROM' not persisted correctly");
+  ASSERT(link.get<iextension::EnergyInterface>() == hits[1], "Link to interface 'TO' not persisted correctly");
+}
+
 template <typename ReaderT>
 int read_frames(const std::string& filename, bool assertBuildVersion = true) {
   auto reader = ReaderT();
@@ -183,6 +227,9 @@ int read_frames(const std::string& filename, bool assertBuildVersion = true) {
     if (reader.currentFileVersion() >= podio::version::Version{0, 99, 99}) {
       checkInterfaceCollection(otherFrame);
     }
+    if (reader.currentFileVersion() >= podio::version::Version{1, 1, 0}) {
+      checkInterfaceExtension(otherFrame);
+    }
   }
 
   if (reader.readNextEntry(podio::Category::Event)) {

tests/root_io/CMakeLists.txt

@@ -25,7 +25,7 @@ if(ENABLE_DATASOURCE)
       read_with_rdatasource_root.cpp
   )
 endif()
-set(root_libs TestDataModelDict ExtensionDataModelDict podio::podioRootIO podio::podioIO)
+set(root_libs TestDataModelDict ExtensionDataModelDict InterfaceExtensionDataModelDict podio::podioRootIO podio::podioIO)
 if(ENABLE_DATASOURCE)
   list(APPEND root_libs podio::podioDataSource)
 endif()

tests/unittests/CMakeLists.txt

@@ -41,7 +41,7 @@ endif()
 
 find_package(Threads REQUIRED)
 add_executable(unittest_podio unittest.cpp frame.cpp buffer_factory.cpp interface_types.cpp std_interoperability.cpp links.cpp)
-target_link_libraries(unittest_podio PUBLIC TestDataModel PRIVATE Catch2::Catch2WithMain Threads::Threads podio::podioRootIO)
+target_link_libraries(unittest_podio PUBLIC TestDataModel InterfaceExtensionDataModel PRIVATE Catch2::Catch2WithMain Threads::Threads podio::podioRootIO)
 if (ENABLE_SIO)
   target_link_libraries(unittest_podio PRIVATE podio::podioSioIO)
 endif()

tests/unittests/interface_types.cpp

@@ -6,6 +6,10 @@
 #include "datamodel/MutableExampleMC.h"
 #include "datamodel/TypeWithEnergy.h"
 
+#include "interface_extension_model/AnotherHit.h"
+#include "interface_extension_model/EnergyInterface.h"
+#include "interface_extension_model/MutableAnotherHit.h"
+
 #include "podio/ObjectID.h"
 #include "podio/utilities/TypeHelpers.h"
 
@@ -131,3 +135,26 @@ TEST_CASE("InterfaceType getters", "[basics][interface-types][code-gen]") {
   TypeWithEnergy interfaceType = cluster;
   REQUIRE(interfaceType.energy() == 3.14f);
 }
+
+TEST_CASE("InterfaceType extension model", "[interface-types][extension]") {
+  using WrapperT = iextension::EnergyInterface;
+
+  auto wrapper = WrapperT::makeEmpty();
+  REQUIRE_FALSE(wrapper.isAvailable());
+
+  MutableExampleCluster cluster{};
+  cluster.energy(3.14f);
+  wrapper = cluster;
+
+  REQUIRE(wrapper.energy() == 3.14f);
+  REQUIRE(wrapper.isA<ExampleCluster>());
+  REQUIRE(wrapper.as<ExampleCluster>().energy() == 3.14f);
+
+  iextension::MutableAnotherHit hit{};
+  hit.energy(4.2f);
+  wrapper = hit;
+
+  REQUIRE(wrapper.energy() == 4.2f);
+  REQUIRE(wrapper.isA<iextension::AnotherHit>());
+  REQUIRE(wrapper.as<iextension::AnotherHit>().energy() == 4.2f);
+}