Skip to content

Commit

Permalink
Make accessing single elements in empty parameter vectors return an e…
Browse files Browse the repository at this point in the history
…mpty optional (AIDASoft#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
  • Loading branch information
jmcarcell committed Nov 18, 2024
1 parent 67186a0 commit f7f93d8
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 0 deletions.
3 changes: 3 additions & 0 deletions include/podio/GenericParameters.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}
Expand Down
7 changes: 7 additions & 0 deletions python/podio/test_Frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions tests/unittests/frame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions tests/unittests/unittest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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{};
Expand Down

0 comments on commit f7f93d8

Please sign in to comment.