Make KeepDropSwitch fail earlier on invalid inputs

CMakeLists.txt

@@ -41,7 +41,7 @@ add_subdirectory(k4FWCore)
 add_subdirectory(k4Interface)
 add_subdirectory(python)
 if(BUILD_TESTING)
-  add_subdirectory(test/k4FWCoreTest)
+  add_subdirectory(test)
 endif()
 
 include(cmake/k4FWCoreCreateConfig.cmake)

k4FWCore/components/IIOSvc.h

@@ -44,7 +44,7 @@ class IIOSvc : virtual public IInterface {
   virtual podio::Writer& getWriter() = 0;
   virtual void deleteWriter() = 0;
   virtual void deleteReader() = 0;
-  virtual bool checkIfWriteCollection(const std::string& collName) = 0;
+  virtual bool checkIfWriteCollection(const std::string& collName) const = 0;
 };
 
 #endif

k4FWCore/components/IOSvc.cpp

@@ -66,7 +66,7 @@ StatusCode IOSvc::initialize() {
 
   m_nextEntry = m_firstEventEntry;
 
-  m_switch = KeepDropSwitch(m_outputCommands);
+  m_switch = k4FWCore::KeepDropSwitch(m_outputCommands);
 
   m_incidentSvc = service("IncidentSvc");
   if (!m_incidentSvc) {
@@ -194,6 +194,6 @@ std::vector<std::string> IOSvc::getAvailableCollections() {
   return {};
 }
 
-bool IOSvc::checkIfWriteCollection(const std::string& collName) { return m_switch.isOn(collName); }
+bool IOSvc::checkIfWriteCollection(const std::string& collName) const { return m_switch.isOn(collName); }
 
 DECLARE_COMPONENT(IOSvc)

k4FWCore/components/IOSvc.h

@@ -64,7 +64,7 @@ class IOSvc : public extends<Service, IIOSvc, IIncidentListener> {
 
   std::mutex m_changeBufferLock;
 
-  KeepDropSwitch m_switch;
+  k4FWCore::KeepDropSwitch m_switch;
 
   std::optional<podio::Reader> m_reader;
   std::optional<podio::Writer> m_writer;
@@ -90,7 +90,7 @@ class IOSvc : public extends<Service, IIOSvc, IIncidentListener> {
   int m_entries{0};
   int m_nextEntry{0};
 
-  bool checkIfWriteCollection(const std::string& collName) override;
+  bool checkIfWriteCollection(const std::string& collName) const override;
 };
 
 #endif

k4FWCore/components/PodioOutput.cpp

@@ -57,7 +57,12 @@ StatusCode PodioOutput::initialize() {
   }
 
   m_framewriter = std::make_unique<podio::ROOTWriter>(m_filename);
-  m_switch = KeepDropSwitch(m_outputCommands);
+  try {
+    m_switch = k4FWCore::KeepDropSwitch(m_outputCommands);
+  } catch (const std::invalid_argument& ex) {
+    fatal() << ex.what() << endmsg;
+    return StatusCode::FAILURE;
+  }
 
   return StatusCode::SUCCESS;
 }

k4FWCore/components/PodioOutput.h

@@ -52,7 +52,7 @@ class [[deprecated("Use the IOSvc instead")]] PodioOutput : public Gaudi::Algori
   Gaudi::Property<std::string> m_filenameRemote{this, "remoteFilename", "",
                                                 "An optional file path to copy the outputfile to."};
   /// Switch for keeping or dropping outputs
-  KeepDropSwitch m_switch;
+  k4FWCore::KeepDropSwitch m_switch;
   PodioDataSvc* m_podioDataSvc;
   /// The actual ROOT frame writer
   std::unique_ptr<podio::ROOTWriter> m_framewriter;

k4FWCore/include/k4FWCore/KeepDropSwitch.h

@@ -23,23 +23,25 @@
 #include <string>
 #include <vector>
 
-std::vector<std::string> split(const std::string& s, char delim);
-
-int wildcmp(const char* wild, const char* string);
+namespace k4FWCore {
 
 class KeepDropSwitch {
 public:
-  enum Cmd { KEEP, DROP, UNKNOWN };
-  typedef std::vector<std::string> CommandLines;
+  typedef std::vector<std::string> InputCommands;
   KeepDropSwitch() = default;
-  explicit KeepDropSwitch(const CommandLines& cmds) { m_commandlines = cmds; }
+  explicit KeepDropSwitch(const InputCommands& cmds);
   bool isOn(const std::string& astring) const;
 
 private:
-  bool getFlag(const std::string& astring) const;
-  Cmd extractCommand(const std::string& cmdLine) const;
-  CommandLines m_commandlines;
+  enum class Cmd { KEEP, DROP, INVALID };
+  using OutputCommand = std::tuple<Cmd, std::string>;
+
+  bool getFlag(const std::string& astring) const noexcept;
+  OutputCommand extractCommand(const std::string& cmdLine) const;
+  std::vector<OutputCommand> m_commandlines{};
   mutable std::map<std::string, bool> m_cache;
 };
 
+} // namespace k4FWCore
+
 #endif

k4FWCore/src/KeepDropSwitch.cpp

@@ -18,10 +18,17 @@
  */
 #include "k4FWCore/KeepDropSwitch.h"
 
-#include <iostream>
 #include <sstream>
 #include <stdexcept>
+#ifdef __cpp_lib_format
+#include <format>
+using std::format;
+#else
+#include <fmt/format.h>
+using fmt::format;
+#endif
 
+namespace {
 int wildcmp(const char* wild, const char* string) {
   // Written by Jack Handy - <A href="mailto:[email protected]">[email protected]</A>
   const char *cp = nullptr, *mp = nullptr;
@@ -63,6 +70,19 @@ std::vector<std::string> split(const std::string& s, char delim) {
   }
   return elems;
 }
+} // namespace
+
+namespace k4FWCore {
+KeepDropSwitch::KeepDropSwitch(const InputCommands& cmds) {
+  m_commandlines.reserve(cmds.size());
+  for (const auto& cmdLine : cmds) {
+    auto [cmd, arg] = extractCommand(cmdLine);
+    if (cmd == Cmd::INVALID) {
+      throw std::invalid_argument(format("'{}' is not a valid command for the KeepDropSwitch", cmdLine));
+    }
+    m_commandlines.emplace_back(cmd, std::move(arg));
+  }
+}
 
 bool KeepDropSwitch::isOn(const std::string& astring) const {
   auto im = m_cache.find(astring);
@@ -75,44 +95,29 @@ bool KeepDropSwitch::isOn(const std::string& astring) const {
   }
 }
 
-bool KeepDropSwitch::getFlag(const std::string& astring) const {
+bool KeepDropSwitch::getFlag(const std::string& astring) const noexcept {
   bool flag = true;
-  for (const auto& cmdline : m_commandlines) {
-    std::vector<std::string> words = split(cmdline, ' ');
-    if (words.size() != 2) {
-      std::ostringstream msg;
-      msg << "malformed command string : " << cmdline;
-      throw std::invalid_argument(msg.str());
-    }
-    std::string cmd = words[0];
-    std::string pattern = words[1];
-    Cmd theCmd = UNKNOWN;
-    if (cmd == "keep")
-      theCmd = KEEP;
-    else if (cmd == "drop")
-      theCmd = DROP;
-    else {
-      std::ostringstream msg;
-      msg << "malformed command in line: " << std::endl;
-      msg << cmdline << std::endl;
-      msg << "should be keep or drop, lower case" << std::endl;
-      throw std::invalid_argument(msg.str());
+  for (const auto& [cmd, pattern] : m_commandlines) {
+    if (wildcmp(pattern.c_str(), astring.c_str())) {
+      flag = (cmd == Cmd::KEEP);
     }
-    bool match = wildcmp(pattern.c_str(), astring.c_str());
-    if (not match)
-      continue;
-    else if (theCmd == KEEP)
-      flag = true;
-    else
-      flag = false;
   }
   return flag;
 }
 
-KeepDropSwitch::Cmd KeepDropSwitch::extractCommand(const std::string& cmdline) const {
+KeepDropSwitch::OutputCommand KeepDropSwitch::extractCommand(const std::string& cmdline) const {
   auto words = split(cmdline, ' ');
-  for (auto& word : words)
-    std::cout << "'" << word << "' ";
-  std::cout << std::endl;
-  return UNKNOWN;
+  if (words.size() != 2) {
+    return {Cmd::INVALID, ""};
+  }
+  if (words[0] == "keep") {
+    return {Cmd::KEEP, words[1]};
+  }
+  if (words[0] == "drop") {
+    return {Cmd::DROP, words[1]};
+  }
+
+  return {Cmd::INVALID, ""};
 }
+
+} // namespace k4FWCore

test/CMakeLists.txt

@@ -0,0 +1,21 @@
+#[[
+Copyright (c) 2014-2024 Key4hep-Project.
+
+This file is part of Key4hep.
+See https://key4hep.github.io/key4hep-doc/ for further info.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+]]
+
+add_subdirectory(k4FWCoreTest)
+add_subdirectory(unittests)

test/k4FWCoreTest/CMakeLists.txt

@@ -150,10 +150,7 @@ add_test_fwcore(TwoProducers options/TwoProducers.py --filename output_k4fwcore_
 add_test_fwcore(CheckCommandLineArguments options/createHelloWorld.py --HelloWorldAlg1.PerEventPrintMessage TwasBrilligAndTheSlithyToves
 PROPERTIES PASS_REGULAR_EXPRESSION "TwasBrilligAndTheSlithyToves"
 )
-add_test(NAME checkKeepDropSwitch
-         WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}
-         COMMAND python scripts/check_KeepDropSwitch.py ${PROJECT_BINARY_DIR}/test/k4FWCoreTest/output_k4test_exampledata_2.root)
-set_property(TEST checkKeepDropSwitch APPEND PROPERTY DEPENDS ReadExampleEventData)
+
 add_test_fwcore(TestUniqueIDGenSvc options/TestUniqueIDGenSvc.py -n 1)
 add_test_fwcore(TestUniqueIDGenSvcRepeated options/TestUniqueIDGenSvc.py -n 2 PROPERTIES PASS_REGULAR_EXPRESSION "Duplicate ID for event number, run number and algorithm name")
 add_test_fwcore(TestEventHeaderFiller options/createEventHeader.py)
@@ -256,6 +253,9 @@ add_test_fwcore(TypeMisMatchDemo options/TypeMisMatchDemo.py PROPERTIES PASS_REG
 add_test_fwcore(TypeMisMatchDemoMultiple options/TypeMisMatchDemoMultiple.py PROPERTIES PASS_REGULAR_EXPRESSION "Failed to cast collection MCParticles1 to the required type")
 add_test_fwcore(ReadMarlinWrapperCollection options/readMarlinWrapperCollection.py)
 
+add_test_fwcore(InvalidOutputCommandsNoCrash options/invalidOutputCommandsNoCrash.py)
+set_tests_properties(InvalidOutputCommandsNoCrash PROPERTIES FIXTURES_REQUIRED ProducerFile PASS_REGULAR_EXPRESSION "ERROR 'abc' is not a valid command for the KeepDropSwitch")
+
 add_test(NAME check_broken_pipe
          COMMAND bash -c "[ $(${K4RUN} options/ExampleFunctionalProducer.py | head -n 2 | wc -l) = 2 ]"
          WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}

test/k4FWCoreTest/scripts/check_KeepDropSwitch.py → test/k4FWCoreTest/options/invalidOutputCommandsNoCrash.py

@@ -1,3 +1,4 @@
+#!/usr/bin/env python3
 #
 # Copyright (c) 2014-2024 Key4hep-Project.
 #
@@ -16,19 +17,25 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-import ROOT
-import sys
 
-ROOT.gSystem.Load("libedm4hepDict")
+from Gaudi.Configuration import INFO
+from k4FWCore import ApplicationMgr, IOSvc
+from Configurables import ExampleFunctionalTransformer, EventDataSvc
 
-file = ROOT.TFile.Open(sys.argv[1])
-tree = file.Get("events")
-tree.GetEntry(0)
+iosvc = IOSvc("IOSvc")
+iosvc.Input = "functional_producer.root"
+iosvc.Output = "invalidOutputCommandsOutput.root"
+iosvc.outputCommands = ["abc"]
 
-ndf = tree.Tracks.at(0).ndf
-if ndf == 0:
-    raise Exception("podio::CollectionBase read from file did not saved properly")
+transformer = ExampleFunctionalTransformer(
+    "Transformer", InputCollection="MCParticles", OutputCollection="NewMCParticles"
+)
 
-status = tree.GetBranchStatus("MCParticles")
-if status:
-    raise Exception("KeepDropSwitch did not drop the collection")
+
+ApplicationMgr(
+    TopAlg=[transformer],
+    EvtSel="NONE",
+    EvtMax=3,
+    ExtSvc=[EventDataSvc("EventDataSvc")],
+    OutputLevel=INFO,
+)

test/unittests/CMakeLists.txt

@@ -0,0 +1,70 @@
+#[[
+Copyright (c) 2014-2024 Key4hep-Project.
+
+This file is part of Key4hep.
+See https://key4hep.github.io/key4hep-doc/ for further info.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+]]
+
+set(USE_EXTERNAL_CATCH2 AUTO CACHE STRING "Link against an external Catch2 v3 static library, otherwise build it locally")
+set_property(CACHE USE_EXTERNAL_CATCH2 PROPERTY STRINGS AUTO ON OFF)
+
+set(CATCH2_MIN_VERSION 3.5.0)
+if(USE_EXTERNAL_CATCH2)
+  if (USE_EXTERNAL_CATCH2 STREQUAL AUTO)
+    find_package(Catch2 ${CATCH2_MIN_VERSION})
+  else()
+    find_package(Catch2 ${CATCH2_MIN_VERSION} REQUIRED)
+  endif()
+endif()
+
+if(NOT Catch2_FOUND)
+  message(STATUS "Fetching local copy of Catch2 library for unit-tests...")
+  # Build Catch2 with the default flags, to avoid generating warnings when we
+  # build it
+  set(CXX_FLAGS_CMAKE_USED ${CMAKE_CXX_FLAGS})
+  set(CMAKE_CXX_FLAGS ${CXX_FLAGS_CMAKE_DEFAULTS})
+  Include(FetchContent)
+  FetchContent_Declare(
+    Catch2
+    GIT_REPOSITORY https://github.com/catchorg/Catch2.git
+    GIT_TAG        v${CATCH2_MIN_VERSION}
+    )
+  FetchContent_MakeAvailable(Catch2)
+  set(CMAKE_MODULE_PATH ${Catch2_SOURCE_DIR}/extras ${CMAKE_MODULE_PATH})
+
+  # Disable clang-tidy on external contents
+  set_target_properties(Catch2 PROPERTIES CXX_CLANG_TIDY "")
+
+  # Hack around the fact, that the include directories are not declared as
+  # SYSTEM for the targets defined this way. Otherwise warnings can still occur
+  # in Catch2 code when templates are evaluated (which happens quite a bit)
+  get_target_property(CATCH2_IF_INC_DIRS Catch2 INTERFACE_INCLUDE_DIRECTORIES)
+  set_target_properties(Catch2 PROPERTIES INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${CATCH2_IF_INC_DIRS}")
+
+  # Reset the flags
+  set(CMAKE_CXX_FLAGS ${CXX_FLAGS_CMAKE_USED})
+endif()
+
+add_executable(unittests_k4FWCore KeepDropSwitchTests.cpp)
+target_link_libraries(unittests_k4FWCore
+  PUBLIC
+  k4FWCore::k4FWCore
+  PRIVATE Catch2::Catch2WithMain
+)
+
+include(Catch)
+catch_discover_tests(unittests_k4FWCore
+  TEST_PREFIX "UT_" # make it possible to filter easily with -R ^UT
+)