Throw an exception instead of ignoring non-existant names

include/podio/SIOFrameData.h

@@ -4,13 +4,10 @@
 #include "podio/CollectionBuffers.h"
 #include "podio/CollectionIDTable.h"
 #include "podio/GenericParameters.h"
-#include "podio/SIOBlock.h"
 
 #include <sio/buffer.h>
 #include <sio/definitions.h>
 
-#include <memory>
-#include <numeric>
 #include <optional>
 #include <string>
 #include <vector>
@@ -36,13 +33,7 @@ class SIOFrameData {
   /// collections. The two size parameters denote the uncompressed size of the
   /// respective buffers.
   SIOFrameData(sio::buffer&& collBuffers, std::size_t dataSize, sio::buffer&& tableBuffer, std::size_t tableSize,
-               std::vector<std::string> limitColls = {}) :
-      m_recBuffer(std::move(collBuffers)),
-      m_tableBuffer(std::move(tableBuffer)),
-      m_dataSize(dataSize),
-      m_tableSize(tableSize),
-      m_limitColls(std::move(limitColls)) {
-  }
+               std::vector<std::string> limitColls = {});
 
   std::optional<podio::CollectionReadBuffers> getCollectionBuffers(const std::string& name);
 

src/RNTupleReader.cc

@@ -158,6 +158,16 @@ std::unique_ptr<ROOTFrameData> RNTupleReader::readEntry(const std::string& categ
     }
   }
 
+  const auto& collInfo = m_collectionInfo[category];
+  // Make sure to not silently ignore non-existant but requested collections
+  if (!collsToRead.empty()) {
+    for (const auto& name : collsToRead) {
+      if (std::ranges::find(collInfo.name, name) == collInfo.name.end()) {
+        throw std::invalid_argument(name + " is not available from Frame");
+      }
+    }
+  }
+
   m_entries[category] = entNum + 1;
 
   // m_readerEntries contains the accumulated entries for all the readers
@@ -178,8 +188,6 @@ std::unique_ptr<ROOTFrameData> RNTupleReader::readEntry(const std::string& categ
   auto dentry = m_readers[category][readerIndex]->GetModel()->GetDefaultEntry();
 #endif
 
-  const auto& collInfo = m_collectionInfo[category];
-
   for (size_t i = 0; i < collInfo.id.size(); ++i) {
     if (!collsToRead.empty() && std::ranges::find(collsToRead, collInfo.name[i]) == collsToRead.end()) {
       continue;

src/ROOTReader.cc

@@ -101,6 +101,15 @@ std::unique_ptr<ROOTFrameData> ROOTReader::readEntry(ROOTReader::CategoryInfo& c
     return nullptr;
   }
 
+  // Make sure to not silently ignore non-existant but requested collections
+  if (!collsToRead.empty()) {
+    for (const auto& name : collsToRead) {
+      if (std::ranges::find(catInfo.storedClasses, name, &detail::NamedCollInfo::name) == catInfo.storedClasses.end()) {
+        throw std::invalid_argument(name + " is not available from Frame");
+      }
+    }
+  }
+
   // After switching trees in the chain, branch pointers get invalidated so
   // they need to be reassigned.
   // NOTE: root 6.22/06 requires that we get completely new branches here,

src/SIOFrameData.cc

@@ -7,6 +7,26 @@
 #include <iterator>
 
 namespace podio {
+
+SIOFrameData::SIOFrameData(sio::buffer&& collBuffers, std::size_t dataSize, sio::buffer&& tableBuffer,
+                           std::size_t tableSize, std::vector<std::string> limitColls) :
+    m_recBuffer(std::move(collBuffers)),
+    m_tableBuffer(std::move(tableBuffer)),
+    m_dataSize(dataSize),
+    m_tableSize(tableSize),
+    m_limitColls(std::move(limitColls)) {
+  readIdTable();
+  // Assuming here that the idTable only contains the collections that are
+  // also available
+  if (!m_limitColls.empty()) {
+    for (const auto& name : m_limitColls) {
+      if (std::ranges::find(m_idTable.names(), name) == m_idTable.names().end()) {
+        throw std::invalid_argument(name + " is not available from Frame");
+      }
+    }
+  }
+}
+
 std::optional<podio::CollectionReadBuffers> SIOFrameData::getCollectionBuffers(const std::string& name) {
   unpackBuffers();
 

tests/read_frame.h

@@ -19,6 +19,7 @@
 #include "podio/Reader.h"
 
 #include <iostream>
+#include <stdexcept>
 
 #define ASSERT(condition, msg)                                                                                         \
   if (!(condition)) {                                                                                                  \
@@ -337,19 +338,25 @@ int test_read_frame_limited(ReaderT& reader) {
     return 1;
   }
 
-  // Check that nothing breaks if we pass in an unavailable collection name
-  const auto emptyEvent = [&]() {
+  // Check that we get the expected exception if trying to read a non-existant
+  // collection
+  try {
     if constexpr (std::is_same_v<ReaderT, podio::Reader>) {
-      return podio::Frame(reader.readFrame("events", 1, {"no-collection-with-this-name", "orThisOne"}));
+      reader.readFrame("events", 1, {"no-collection-with-this-name", "orThisOne"});
     } else {
-      return podio::Frame(reader.readEntry("events", 1, {"no-collection-with-this-name", "orThisOne"}));
+      reader.readEntry("events", 1, {"no-collection-with-this-name", "orThisOne"});
     }
-  }();
-
-  if (!emptyEvent.getAvailableCollections().empty()) {
-    std::cerr << "Trying to read collection names that are unavailable should not break, but should also not give a "
-                 "meaningful event"
-              << std::endl;
+    // if we haven't gotten an exception before we fail
+    std::cerr << "Attempting to read non-existant collections should result in an exception" << std::endl;
+    return 1;
+  } catch (std::invalid_argument& ex) {
+    if (ex.what() != std::string("no-collection-with-this-name is not available from Frame")) {
+      std::cerr << "Exception message for attempting to limit to non-existant collections not as expected: "
+                << ex.what() << std::endl;
+      return 1;
+    }
+  } catch (...) {
+    std::cerr << "Attempting to read a non-existant collection didn't yield the correct exception" << std::endl;
     return 1;
   }