logging: Simplify rotating file sink

include/hage/logging/rotating_file_sink.hpp

@@ -3,6 +3,7 @@
 #include <filesystem>
 #include <functional>
 
+#include <deque>
 #include <hage/core/concepts.hpp>
 
 #include "sink.hpp"
@@ -14,85 +15,49 @@ struct LogFileStats final
   std::size_t bytes;
 };
 
-// We should create a concept
-template<typename T>
-concept Splitter = std::predicate<T, const LogFileStats&>;
+// We want to support two modes: maxSize
+// time stamps.
 
-class SizeSplitter final
+class Rotater
 {
 public:
-  explicit SizeSplitter(std::size_t sz)
-    : m_maxSize{ sz }
+  enum class Type
   {
-  }
+    MoveBackwards,
+    RemoveLast
+  };
 
-  [[nodiscard]] constexpr bool operator()(const LogFileStats& stats) const { return m_maxSize < stats.bytes; }
+  virtual ~Rotater() = default;
 
-private:
-  std::size_t m_maxSize;
-};
-static_assert(Splitter<SizeSplitter>);
+  [[nodiscard]] virtual bool shouldRotate(const LogFileStats& stats) = 0;
+  [[nodiscard]] virtual std::vector<std::filesystem::path> generateNames(const LogFileStats& stats, int times) = 0;
+  [[nodiscard]] virtual Type getRotateType() const = 0;
 
-template<Splitter... Splitters>
-class AndSplitter final
-{
-public:
-  explicit AndSplitter(Splitters&&... splitters)
-    : m_splitters{ std::forward<Splitters>(splitters)... }
-  {
-  }
-
-  [[nodiscard]] bool operator()(const LogFileStats& stats)
-  {
-    return [this, &stats]<std::size_t... Is>(std::index_sequence<Is...>) {
-      return (... and std::get<Is>(m_splitters)(stats));
-    }(std::index_sequence_for<Splitters...>{});
-  }
+  // disable assignment operator (due to the problem of slicing):
+  Rotater& operator=(Rotater&&) = delete;
+  Rotater& operator=(const Rotater&) = delete;
 
-private:
-  std::tuple<Splitters...> m_splitters;
+protected:
+  Rotater() = default;
+  // enable copy and move semantics (callable only for derived classes):
+  Rotater(const Rotater&) = default;
+  Rotater(Rotater&&) = default;
 };
 
-static_assert(Splitter<AndSplitter<SizeSplitter, SizeSplitter>>);
-
-template<Splitter... Splitters>
-class OrSplitter final
+class SizeRotater final : public Rotater
 {
 public:
-  explicit OrSplitter(Splitters&&... splitters)
-    : m_splitters{ std::forward<Splitters>(splitters)... }
-  {
-  }
+  SizeRotater(std::filesystem::path base, std::size_t maxSize);
 
-  [[nodiscard]] bool operator()(const LogFileStats& stats)
-  {
-    return [this, &stats]<std::size_t... Is>(std::index_sequence<Is...>) {
-      return (... or std::get<Is>(m_splitters)(stats));
-    }(std::index_sequence_for<Splitters...>{});
-  }
+  [[nodiscard]] bool shouldRotate(const LogFileStats& stats) override;
+  [[nodiscard]] std::vector<std::filesystem::path> generateNames(const LogFileStats& stats, int times) override;
+  [[nodiscard]] Type getRotateType() const override { return Type::MoveBackwards; };
 
 private:
-  std::tuple<Splitters...> m_splitters;
-};
-static_assert(Splitter<OrSplitter<SizeSplitter, SizeSplitter>>);
-
-// We create a namer, with a removal policy.
-class Namer
-{
-public:
-  virtual ~Namer() = default;
-
-  [[nodiscard]] virtual std::vector<std::filesystem::path> generateNames(const LogFileStats& stats, int backlog) = 0;
-
-  // disable assignment operator (due to the problem of slicing):
-  Namer& operator=(Namer&&) = delete;
-  Namer& operator=(const Namer&) = delete;
+  std::size_t m_maxSize;
+  std::filesystem::path m_base;
 
-protected:
-  Namer() = default;
-  // enable copy and move semantics (callable only for derived classes):
-  Namer(const Namer&) = default;
-  Namer(Namer&&) = default;
+  std::vector<std::filesystem::path> m_names;
 };
 
 class RotatingFileSink final : public Sink
@@ -101,25 +66,21 @@ class RotatingFileSink final : public Sink
   struct Config final
   {
     std::filesystem::path saveDirectory;
-    int backlog{ 10 };
 
     [[nodiscard]] bool valid() const
     {
-      bool good = (0 <= backlog);
-      good = good && !saveDirectory.empty();
+      bool good = !saveDirectory.empty();
 
       return good;
     }
   };
 
   void receive(LogLevel level, const timestamp_type& ts, std::string_view line) override;
 
-  template<Splitter T>
-  RotatingFileSink(Config conf, T&& splitter)
+  RotatingFileSink(Config conf, std::unique_ptr<Rotater> rotater)
     : m_conf{ std::move(conf) }
-    , m_splitter(std::forward<T>(splitter))
+    , m_rotater(std::move(rotater))
   {
-
     if (m_conf.valid()) {
       throw std::runtime_error("Invalid configuration");
     }
@@ -129,7 +90,9 @@ class RotatingFileSink final : public Sink
 
 private:
   Config m_conf;
-  std::function<bool(const LogFileStats&)> m_splitter;
+  std::unique_ptr<Rotater> m_rotater;
+
+  std::deque<std::filesystem::path> m_prevNames;
 };
 
 }

src/logging/rotating_file_sink.cpp

@@ -1,10 +1,45 @@
+#include "fmt/core.h"
+#include "fmt/std.h"
+
 #include <hage/logging/rotating_file_sink.hpp>
+
+using namespace hage;
+
 void
-hage::RotatingFileSink::receive(hage::LogLevel level, const hage::Sink::timestamp_type& ts, std::string_view line)
+RotatingFileSink::receive(LogLevel level, const Sink::timestamp_type& ts, std::string_view line)
 {
   std::ignore = level;
   std::ignore = ts;
   std::ignore = line;
 }
 
-hage::RotatingFileSink::~RotatingFileSink() {}
+RotatingFileSink::~RotatingFileSink() {}
+
+bool
+SizeRotater::shouldRotate(const LogFileStats& stats)
+{
+  return m_maxSize <= stats.bytes;
+}
+
+std::vector<std::filesystem::path>
+SizeRotater::generateNames(const LogFileStats& stats, int times)
+{
+  if (times <= 0)
+    return {};
+
+  // We are simply delivering the messages.
+  std::vector<std::filesystem::path> ans;
+  ans.push_back(m_base);
+
+  for (int i = 1; i < times; i++) {
+    ans.emplace_back(fmt::format("{}.{}", m_base, i));
+  }
+
+  return ans;
+}
+
+SizeRotater::SizeRotater(std::filesystem::path base, std::size_t maxSize)
+  : m_maxSize{ maxSize }
+  , m_base{ std::move(base) }
+{
+}

tests/rotating_file_sink_tests.cpp

@@ -4,84 +4,47 @@
 
 #include <hage/logging/rotating_file_sink.hpp>
 
-namespace {
-
-struct FalseSplitter
-{
-  [[nodiscard]] constexpr bool operator()(const hage::LogFileStats&) { return false; }
-};
-
-struct TrueSplitter
-{
-  [[nodiscard]] constexpr bool operator()(const hage::LogFileStats&) { return true; }
-};
-}
-
 TEST_SUITE_BEGIN("logging");
 
-TEST_CASE("AndSplitter")
+TEST_CASE("SizeRotater")
 {
-  hage::LogFileStats exampleStats1{ .bytes = 10 };
+  hage::SizeRotater rotater{ "test.log", 100 };
+  hage::LogFileStats underLimitStat{ .bytes = 10 };
+  hage::LogFileStats overLimitStat{ .bytes = 150 };
 
-  SUBCASE("Should handle more true")
-  {
-    hage::AndSplitter splitter{ TrueSplitter{}, TrueSplitter{} };
+  REQUIRE_EQ(rotater.getRotateType(), hage::Rotater::Type::MoveBackwards);
 
-    REQUIRE_UNARY(splitter(exampleStats1));
-  }
-
-  SUBCASE("No splitters should produce true")
+  SUBCASE("Should not break on smaller writing")
   {
-    hage::AndSplitter splitter{};
-
-    REQUIRE_UNARY(splitter(exampleStats1));
+    CHECK_UNARY_FALSE(rotater.shouldRotate(underLimitStat));
   }
 
-  SUBCASE("All false should produce false")
+  SUBCASE("Should break bigger files")
   {
-    hage::AndSplitter splitter{ FalseSplitter{}, FalseSplitter{} };
-
-    REQUIRE_UNARY_FALSE(splitter(exampleStats1));
+    CHECK_UNARY(rotater.shouldRotate(overLimitStat));
   }
 
-  SUBCASE("One false should produce false")
+  SUBCASE("Zero backlog, should be empty")
   {
-    hage::AndSplitter splitter{ TrueSplitter{}, FalseSplitter{} };
-
-    REQUIRE_UNARY_FALSE(splitter(exampleStats1));
+    CHECK_UNARY(rotater.generateNames(underLimitStat, 0).empty());
   }
-}
 
-TEST_CASE("OrSplitter")
-{
-  hage::LogFileStats exampleStats1{ .bytes = 10 };
-
-  SUBCASE("All true should be true")
-  {
-    hage::OrSplitter splitter{ TrueSplitter{}, TrueSplitter{} };
-
-    REQUIRE_UNARY(splitter(exampleStats1));
-  }
-
-  SUBCASE("No splitters should produce false")
+  SUBCASE("One backlog, should return just the base filename")
   {
-    hage::OrSplitter splitter;
-
-    REQUIRE_UNARY_FALSE(splitter(exampleStats1));
-  }
-
-  SUBCASE("All false should produce false")
-  {
-    hage::OrSplitter splitter{ FalseSplitter{}, FalseSplitter{} };
-
-    REQUIRE_UNARY_FALSE(splitter(exampleStats1));
+    const auto names = rotater.generateNames(underLimitStat, 1);
+    CHECK_EQ(names.size(), 1);
+    CHECK_EQ(names[0], "test.log");
   }
 
-  SUBCASE("One false should produce true")
+  SUBCASE("More backlog, should return the right patterns")
   {
-    hage::OrSplitter splitter{ TrueSplitter{}, FalseSplitter{} };
-
-    REQUIRE_UNARY(splitter(exampleStats1));
+    const auto names = rotater.generateNames(underLimitStat, 5);
+    CHECK_EQ(names.size(), 5);
+    CHECK_EQ(names[0], "test.log");
+    CHECK_EQ(names[1], "test.log.1");
+    CHECK_EQ(names[2], "test.log.2");
+    CHECK_EQ(names[3], "test.log.3");
+    CHECK_EQ(names[4], "test.log.4");
   }
 }