From c9900288c5457a566a8c0690e918511eaa86aabc Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Tue, 3 Sep 2024 15:19:46 +0200 Subject: [PATCH 1/6] check metadata access with optional in the tests --- .../src/components/k4FWCoreTest_cellID_reader.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp b/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp index dcc37779..1a28305d 100644 --- a/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp +++ b/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp @@ -39,5 +39,17 @@ StatusCode k4FWCoreTest_cellID_reader::execute(const EventContext&) const { return StatusCode::FAILURE; } + auto optCellIDstr = m_cellIDHandle.get_optional(); + + if (!optCellIDstr.has_value()) { + error() << "ERROR cellID is empty but was expected to hold a value" << endmsg; + return StatusCode::FAILURE; + } + + if (optCellIDstr.value() != cellIDstr) { + error() << "ERROR metadata accessed with by optional and value differs" << endmsg; + return StatusCode::FAILURE; + } + return StatusCode::SUCCESS; } From eeaac4cc52628adc8aa6399987f7b477be02f22f Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Tue, 3 Sep 2024 14:01:35 +0200 Subject: [PATCH 2/6] don't create metadatasvc when checking if it exists --- k4FWCore/include/k4FWCore/MetaDataHandle.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/k4FWCore/include/k4FWCore/MetaDataHandle.h b/k4FWCore/include/k4FWCore/MetaDataHandle.h index c923b785..cf1aeaf3 100644 --- a/k4FWCore/include/k4FWCore/MetaDataHandle.h +++ b/k4FWCore/include/k4FWCore/MetaDataHandle.h @@ -159,12 +159,8 @@ template void MetaDataHandle::checkPodioDataSvc() { if (cmd.find("genconf") != std::string::npos) return; - // The proper check would be the following: - // if (!m_podio_data_service && !Gaudi::svcLocator()->service("MetadataSvc")) { - // However, it seems there is always a service called "MetadataSvc" from Gaudi, - // so the check will always pass - if (!m_podio_data_service) { - std::cout << "Warning: MetaDataHandles require the PodioDataSvc (ignore if using IOSvc)" << std::endl; + if (!m_podio_data_service && !Gaudi::svcLocator()->service("MetadataSvc", false)) { + std::cout << "Warning: MetaDataHandles require the PodioDataSvc or for compatibility the MetadataSvc" << std::endl; } } From b03288d2a730b6d5996ae8e1b5654a5e4d062bdd Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Tue, 3 Sep 2024 14:03:15 +0200 Subject: [PATCH 3/6] check if PodioDataSvc exists in get_optional --- k4FWCore/include/k4FWCore/MetaDataHandle.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/k4FWCore/include/k4FWCore/MetaDataHandle.h b/k4FWCore/include/k4FWCore/MetaDataHandle.h index cf1aeaf3..b7ed448c 100644 --- a/k4FWCore/include/k4FWCore/MetaDataHandle.h +++ b/k4FWCore/include/k4FWCore/MetaDataHandle.h @@ -89,8 +89,10 @@ MetaDataHandle::MetaDataHandle(const Gaudi::DataHandle& handle, const std::st //--------------------------------------------------------------------------- template std::optional MetaDataHandle::get_optional() const { - const auto& frame = m_podio_data_service->getMetaDataFrame(); - return frame.getParameter(fullDescriptor()); + if (m_podio_data_service) { + return m_podio_data_service->getMetaDataFrame().getParameter(fullDescriptor()); + } + return k4FWCore::getParameter(fullDescriptor()); } //--------------------------------------------------------------------------- From f575a379a21831e969fd194f226f5532906f1021 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Tue, 3 Sep 2024 14:05:42 +0200 Subject: [PATCH 4/6] fix unchecked optional for MetaDataHandle using IOSvc --- k4FWCore/include/k4FWCore/MetaDataHandle.h | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/k4FWCore/include/k4FWCore/MetaDataHandle.h b/k4FWCore/include/k4FWCore/MetaDataHandle.h index b7ed448c..4491eec9 100644 --- a/k4FWCore/include/k4FWCore/MetaDataHandle.h +++ b/k4FWCore/include/k4FWCore/MetaDataHandle.h @@ -97,19 +97,12 @@ template std::optional MetaDataHandle::get_optional() const { //--------------------------------------------------------------------------- template const T MetaDataHandle::get() const { - std::optional maybeVal; - // DataHandle based algorithms - if (m_podio_data_service) { - maybeVal = get_optional(); - if (!maybeVal.has_value()) { - throw GaudiException("MetaDataHandle empty handle access", - "MetaDataHandle " + fullDescriptor() + " not (yet?) available", StatusCode::FAILURE); - } - // Functional algorithms - } else { - maybeVal = k4FWCore::getParameter(fullDescriptor()); + auto optional_parameter = get_optional(); + if (!optional_parameter.has_value()) { + throw GaudiException("MetaDataHandle empty handle access", + "MetaDataHandle " + fullDescriptor() + " not (yet?) available", StatusCode::FAILURE); } - return maybeVal.value(); + return optional_parameter.value(); } //--------------------------------------------------------------------------- From 7e10b2fd26cf921292dc357b4bd679ec273d7fb0 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Tue, 3 Sep 2024 14:46:04 +0200 Subject: [PATCH 5/6] remove unused declared but not defined constructor --- k4FWCore/include/k4FWCore/MetaDataHandle.h | 1 - 1 file changed, 1 deletion(-) diff --git a/k4FWCore/include/k4FWCore/MetaDataHandle.h b/k4FWCore/include/k4FWCore/MetaDataHandle.h index 4491eec9..0a906428 100644 --- a/k4FWCore/include/k4FWCore/MetaDataHandle.h +++ b/k4FWCore/include/k4FWCore/MetaDataHandle.h @@ -26,7 +26,6 @@ template class MetaDataHandle { public: - MetaDataHandle(); MetaDataHandle(const std::string& descriptor, Gaudi::DataHandle::Mode a); MetaDataHandle(const Gaudi::DataHandle& handle, const std::string& descriptor, Gaudi::DataHandle::Mode a); From 0c36f305d2c7023d21464301bdef6688c8d571e3 Mon Sep 17 00:00:00 2001 From: Juan Miguel Carceller <22276694+jmcarcell@users.noreply.github.com> Date: Wed, 4 Sep 2024 16:52:31 +0200 Subject: [PATCH 6/6] Apply suggestions from code review --- .../src/components/k4FWCoreTest_cellID_reader.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp b/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp index 1a28305d..9d48733c 100644 --- a/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp +++ b/test/k4FWCoreTest/src/components/k4FWCoreTest_cellID_reader.cpp @@ -42,12 +42,12 @@ StatusCode k4FWCoreTest_cellID_reader::execute(const EventContext&) const { auto optCellIDstr = m_cellIDHandle.get_optional(); if (!optCellIDstr.has_value()) { - error() << "ERROR cellID is empty but was expected to hold a value" << endmsg; + error() << "ERROR: cellID is empty but was expected to hold a value" << endmsg; return StatusCode::FAILURE; } if (optCellIDstr.value() != cellIDstr) { - error() << "ERROR metadata accessed with by optional and value differs" << endmsg; + error() << "ERROR: metadata accessed with by optional and value differs" << endmsg; return StatusCode::FAILURE; }