From f7f93d83ade12a3ed8ad752532d8a6c90b084a76 Mon Sep 17 00:00:00 2001
From: jmcarcell <jmcarcell@users.noreply.github.com>
Date: Mon, 18 Nov 2024 18:42:37 +0100
Subject: [PATCH] Make accessing single elements in empty parameter vectors
 return an empty optional (#693)

* Add simple reproducer of previous issue as test

* Return an empty optional in case there are no values

Otherwise we run into a read out of bounds in case the parameter is
stored, but with an empty vector value. E.g. from setting it with a
vector that comes from an external source which is empty.

* Add test for python bindings
---
 include/podio/GenericParameters.h |  3 +++
 python/podio/test_Frame.py        |  7 +++++++
 tests/unittests/frame.cpp         |  6 ++++++
 tests/unittests/unittest.cpp      | 15 +++++++++++++++
 4 files changed, 31 insertions(+)

diff --git a/include/podio/GenericParameters.h b/include/podio/GenericParameters.h
index d0b3b3d26..722e8d299 100644
--- a/include/podio/GenericParameters.h
+++ b/include/podio/GenericParameters.h
@@ -195,6 +195,9 @@ std::optional<T> GenericParameters::get(const std::string& key) const {
     return it->second;
   } else {
     const auto& iv = it->second;
+    if (iv.empty()) {
+      return std::nullopt;
+    }
     return iv[0];
   }
 }
diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py
index b28a5b105..77373dcf6 100644
--- a/python/podio/test_Frame.py
+++ b/python/podio/test_Frame.py
@@ -138,6 +138,13 @@ def test_frame_put_parameters(self):
         frame.put_parameter("float_as_float", 3.14, as_type="float")
         self.assertAlmostEqual(frame.get_parameter("float_as_float"), 3.14, places=5)
 
+    def test_frame_empty_parameters(self):
+        """Check that working with empty parameters works"""
+        frame = Frame()
+        frame.put_parameter("empty_param", [], as_type="int")
+        vals = frame.get_parameter("empty_param")
+        self.assertEqual(len(vals), 0)
+
 
 class FrameReadTest(unittest.TestCase):
     """Unit tests for the Frame python bindings for Frames read from file.
diff --git a/tests/unittests/frame.cpp b/tests/unittests/frame.cpp
index ae808940c..dc02d5de7 100644
--- a/tests/unittests/frame.cpp
+++ b/tests/unittests/frame.cpp
@@ -50,6 +50,12 @@ TEST_CASE("Frame parameters", "[frame][basics]") {
   // Can't rely on an insertion order here
   REQUIRE(std::ranges::find(stringKeys, "aString") != stringKeys.end());
   REQUIRE(std::ranges::find(stringKeys, "someStrings") != stringKeys.end());
+
+  // Check the cases with empty vectors as parameters
+  event.putParameter("emptyVec", std::vector<int>{});
+  const auto emptyVec = event.getParameter<std::vector<int>>("emptyVec").value();
+  REQUIRE(emptyVec.empty());
+  REQUIRE_FALSE(event.getParameter<int>("emptyVec").has_value());
 }
 
 // NOTE: Due to the extremely small tasks that are done in these tests, they will
diff --git a/tests/unittests/unittest.cpp b/tests/unittests/unittest.cpp
index 5f110ded8..0e0210915 100644
--- a/tests/unittests/unittest.cpp
+++ b/tests/unittests/unittest.cpp
@@ -1070,6 +1070,21 @@ TEST_CASE("GenericParameters", "[generic-parameters]") {
   REQUIRE_FALSE(gp.get<std::vector<std::string>>("Missing"));
 }
 
+TEST_CASE("GenericParameters empty vector single value access", "[generic-parameters]") {
+  auto gp = podio::GenericParameters();
+  gp.set("empty-ints", std::vector<int>{});
+
+  // Getting the whole vector works
+  const auto maybeVec = gp.get<std::vector<int>>("empty-ints");
+  REQUIRE(maybeVec.has_value());
+  const auto& vec = maybeVec.value();
+  REQUIRE(vec.empty());
+
+  // Trying to get a single (i.e. the first) value will not work
+  const auto maybeVal = gp.get<int>("empty-ints");
+  REQUIRE_FALSE(maybeVal.has_value());
+}
+
 TEST_CASE("GenericParameters constructors", "[generic-parameters]") {
   // Tests for making sure that generic parameters can be moved / copied correctly
   auto originalParams = podio::GenericParameters{};