fix: Better errors on logger init failure (#1857)

Fixes #1326.

src/main/Main.cpp

@@ -52,7 +52,10 @@ try {
             if (not app::parseConfig(run.configPath))
                 return EXIT_FAILURE;
 
-            util::LogService::init(gClioConfig);
+            if (auto const initSuccess = util::LogService::init(gClioConfig); not initSuccess) {
+                std::cerr << initSuccess.error() << std::endl;
+                return EXIT_FAILURE;
+            }
             app::ClioApplication clio{gClioConfig};
             return clio.run(run.useNgWebServer);
         },
@@ -60,7 +63,10 @@ try {
             if (not app::parseConfig(migrate.configPath))
                 return EXIT_FAILURE;
 
-            util::LogService::init(gClioConfig);
+            if (auto const initSuccess = util::LogService::init(gClioConfig); not initSuccess) {
+                std::cerr << initSuccess.error() << std::endl;
+                return EXIT_FAILURE;
+            }
             app::MigratorApplication migrator{gClioConfig, migrate.subCmd};
             return migrator.run();
         }

src/util/log/Logger.cpp

@@ -28,8 +28,6 @@
 
 #include <boost/algorithm/string/predicate.hpp>
 #include <boost/date_time/posix_time/posix_time_duration.hpp>
-#include <boost/filesystem/operations.hpp>
-#include <boost/filesystem/path.hpp>
 #include <boost/log/attributes/attribute_value_set.hpp>
 #include <boost/log/core/core.hpp>
 #include <boost/log/expressions/filter.hpp>
@@ -48,18 +46,20 @@
 #include <boost/log/utility/setup/console.hpp>
 #include <boost/log/utility/setup/file.hpp>
 #include <boost/log/utility/setup/formatter_parser.hpp>
+#include <fmt/core.h>
 
 #include <algorithm>
 #include <array>
 #include <cstddef>
 #include <cstdint>
+#include <filesystem>
 #include <ios>
 #include <iostream>
 #include <optional>
 #include <ostream>
-#include <stdexcept>
 #include <string>
 #include <string_view>
+#include <system_error>
 #include <unordered_map>
 #include <utility>
 
@@ -111,7 +111,7 @@ getSeverityLevel(std::string_view logLevel)
     std::unreachable();
 }
 
-void
+std::expected<void, std::string>
 LogService::init(config::ClioConfigDefinition const& config)
 {
     namespace keywords = boost::log::keywords;
@@ -132,9 +132,15 @@ LogService::init(config::ClioConfigDefinition const& config)
 
     auto const logDir = config.maybeValue<std::string>("log_directory");
     if (logDir) {
-        boost::filesystem::path dirPath{logDir.value()};
-        if (!boost::filesystem::exists(dirPath))
-            boost::filesystem::create_directories(dirPath);
+        std::filesystem::path dirPath{logDir.value()};
+        if (not std::filesystem::exists(dirPath)) {
+            if (std::error_code error; not std::filesystem::create_directories(dirPath, error)) {
+                return std::unexpected{
+                    fmt::format("Couldn't create logs directory '{}': {}", dirPath.string(), error.message())
+                };
+            }
+        }
+
         auto const rotationPeriod = config.get<uint32_t>("log_rotation_hour_interval");
 
         // the below are taken from user in MB, but boost::log::add_file_log needs it to be in bytes
@@ -169,8 +175,9 @@ LogService::init(config::ClioConfigDefinition const& config)
     for (auto it = overrides.begin<util::config::ObjectView>(); it != overrides.end<util::config::ObjectView>(); ++it) {
         auto const& channelConfig = *it;
         auto const name = channelConfig.get<std::string>("channel");
-        if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0)
-            throw std::runtime_error("Can't override settings for log channel " + name + ": invalid channel");
+        if (std::ranges::count(Logger::kCHANNELS, name) == 0) { // TODO: use std::ranges::contains when available
+            return std::unexpected{fmt::format("Can't override settings for log channel {}: invalid channel", name)};
+        }
 
         minSeverity[name] = getSeverityLevel(channelConfig.get<std::string>("log_level"));
     }
@@ -189,6 +196,7 @@ LogService::init(config::ClioConfigDefinition const& config)
     filter = boost::log::filter{std::move(logFilter)};
     boost::log::core::get()->set_filter(filter);
     LOG(LogService::info()) << "Default log level = " << defaultSeverity;
+    return {};
 }
 
 Logger::Pump

src/util/log/Logger.hpp

@@ -46,6 +46,7 @@
 
 #include <array>
 #include <cstddef>
+#include <expected>
 #include <optional>
 #include <ostream>
 #include <string>
@@ -278,8 +279,9 @@ public:
      * @brief Global log core initialization from a @ref Config
      *
      * @param config The configuration to use
+     * @return Void on success, error message on failure
      */
-    static void
+    [[nodiscard]] static std::expected<void, std::string>
     init(config::ClioConfigDefinition const& config);
 
     /**

src/util/newconfig/ConfigDefinition.hpp

@@ -345,10 +345,10 @@ static ClioConfigDefinition gClioConfig = ClioConfigDefinition{
      {"cache.page_fetch_size", ConfigValue{ConfigType::Integer}.defaultValue(512).withConstraint(gValidateUint16)},
      {"cache.load", ConfigValue{ConfigType::String}.defaultValue("async").withConstraint(gValidateLoadMode)},
 
-     {"log_channels.[].channel", Array{ConfigValue{ConfigType::String}.optional().withConstraint(gValidateChannelName)}
+     {"log_channels.[].channel", Array{ConfigValue{ConfigType::String}.withConstraint(gValidateChannelName)}
      },
      {"log_channels.[].log_level",
-      Array{ConfigValue{ConfigType::String}.optional().withConstraint(gValidateLogLevelName)}},
+      Array{ConfigValue{ConfigType::String}.withConstraint(gValidateLogLevelName)}},
 
      {"log_level", ConfigValue{ConfigType::String}.defaultValue("info").withConstraint(gValidateLogLevelName)},
 

tests/unit/LoggerTests.cpp

@@ -26,9 +26,11 @@
 #include "util/newconfig/ConfigValue.hpp"
 #include "util/newconfig/Types.hpp"
 
+#include <boost/json/array.hpp>
 #include <boost/json/object.hpp>
 #include <boost/json/parse.hpp>
 #include <fmt/core.h>
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include <cstddef>
@@ -113,7 +115,7 @@ TEST_F(LoggerInitTest, DefaultLogLevel)
     ASSERT_FALSE(parsingErrors.has_value());
     std::string const logString = "some log";
 
-    LogService::init(config_);
+    EXPECT_TRUE(LogService::init(config_));
     for (auto const& channel : Logger::kCHANNELS) {
         Logger const log{channel};
         log.trace() << logString;
@@ -151,7 +153,7 @@ TEST_F(LoggerInitTest, ChannelLogLevel)
     ASSERT_FALSE(parsingErrors.has_value());
     std::string const logString = "some log";
 
-    LogService::init(config_);
+    EXPECT_TRUE(LogService::init(config_));
     for (auto const& channel : Logger::kCHANNELS) {
         Logger const log{channel};
         log.trace() << logString;
@@ -175,6 +177,38 @@ TEST_F(LoggerInitTest, ChannelLogLevel)
     }
 }
 
+TEST_F(LoggerInitTest, InitReturnsErrorIfCouldNotCreateLogDirectory)
+{
+    // "/proc" directory is read only on any unix OS
+    auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{{"log_directory", "/proc/logs"}}});
+    ASSERT_FALSE(parsingErrors.has_value());
+
+    auto const result = LogService::init(config_);
+    EXPECT_FALSE(result);
+    EXPECT_THAT(result.error(), testing::HasSubstr("Couldn't create logs directory"));
+}
+
+TEST_F(LoggerInitTest, InitReturnsErrorIfProvidedInvalidChannel)
+{
+    auto const jsonStr = R"json(
+    {
+        "log_channels": [
+            {
+                "channel": "SomeChannel",
+                "log_level": "warn"
+            }
+        ]
+    })json";
+
+    auto const json = boost::json::parse(jsonStr).as_object();
+    auto const parsingErrors = config_.parse(ConfigFileJson{json});
+    ASSERT_FALSE(parsingErrors.has_value());
+
+    auto const result = LogService::init(config_);
+    EXPECT_FALSE(result);
+    EXPECT_EQ(result.error(), "Can't override settings for log channel SomeChannel: invalid channel");
+}
+
 TEST_F(LoggerInitTest, LogSizeAndHourRotationCannotBeZero)
 {
     std::vector<std::string_view> const keys{