From: Joel Rosdahl <joel@rosdahl.net>
Date: Mon, 15 Aug 2022 19:10:39 +0000 (+0200)
Subject: refactor: Use util::Blob for binary data in storage API
X-Git-Tag: v4.7~113
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=6288b53c037176c6931a3232d983e65782e79e68;p=thirdparty%2Fccache.git

refactor: Use util::Blob for binary data in storage API
---

diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp
index b0c06ecc9..d56f8ec8b 100644
--- a/src/storage/Storage.cpp
+++ b/src/storage/Storage.cpp
@@ -36,6 +36,7 @@
 #include <util/Tokenizer.hpp>
 #include <util/XXH3_64.hpp>
 #include <util/expected.hpp>
+#include <util/file.hpp>
 #include <util/string.hpp>
 
 #include <third_party/url.hpp>
@@ -248,14 +249,12 @@ Storage::get(const Digest& key,
           m_secondary_storages.end(),
           [](const auto& entry) { return !entry->config.read_only; });
         if (should_put_in_secondary_storage) {
-          std::string value;
-          try {
-            value = Util::read_file(*path);
-          } catch (const core::Error& e) {
-            LOG("Failed to read {}: {}", *path, e.what());
+          const auto value = util::read_file<util::Blob>(*path);
+          if (!value) {
+            LOG("Failed to read {}: {}", *path, value.error());
             return path; // Don't indicate failure since primary storage was OK.
           }
-          put_in_secondary_storage(key, value, true);
+          put_in_secondary_storage(key, *value, true);
         }
       }
 
@@ -277,7 +276,7 @@ Storage::get(const Digest& key,
   TemporaryFile tmp_file(FMT("{}/tmp.get", m_config.temporary_dir()));
   m_tmp_files.push_back(tmp_file.path);
   try {
-    Util::write_file(tmp_file.path, value);
+    util::write_file(tmp_file.path, value);
   } catch (const core::Error& e) {
     throw core::Fatal("Error writing to {}: {}", tmp_file.path, e.what());
   }
@@ -317,14 +316,12 @@ Storage::put(const Digest& key,
                 m_secondary_storages.end(),
                 [](const auto& entry) { return !entry->config.read_only; });
   if (should_put_in_secondary_storage) {
-    std::string value;
-    try {
-      value = Util::read_file(*path);
-    } catch (const core::Error& e) {
-      LOG("Failed to read {}: {}", *path, e.what());
+    const auto value = util::read_file<util::Blob>(*path);
+    if (!value) {
+      LOG("Failed to read {}: {}", *path, value.error());
       return true; // Don't indicate failure since primary storage was OK.
     }
-    put_in_secondary_storage(key, value, false);
+    put_in_secondary_storage(key, *value, false);
   }
 
   return true;
@@ -479,7 +476,7 @@ Storage::get_backend(SecondaryStorageEntry& entry,
   }
 }
 
-std::optional<std::pair<std::string, bool>>
+std::optional<std::pair<util::Blob, bool>>
 Storage::get_from_secondary_storage(const Digest& key)
 {
   MTR_SCOPE("secondary_storage", "get");
@@ -520,7 +517,7 @@ Storage::get_from_secondary_storage(const Digest& key)
 
 void
 Storage::put_in_secondary_storage(const Digest& key,
-                                  const std::string& value,
+                                  const util::Blob& value,
                                   bool only_if_missing)
 {
   MTR_SCOPE("secondary_storage", "put");
diff --git a/src/storage/Storage.hpp b/src/storage/Storage.hpp
index 4a18aeeb2..5d9abd0bf 100644
--- a/src/storage/Storage.hpp
+++ b/src/storage/Storage.hpp
@@ -22,6 +22,7 @@
 #include <storage/primary/PrimaryStorage.hpp>
 #include <storage/secondary/SecondaryStorage.hpp>
 #include <storage/types.hpp>
+#include <util/types.hpp>
 
 #include <functional>
 #include <memory>
@@ -81,11 +82,11 @@ private:
               const Digest& key,
               std::string_view operation_description,
               const bool for_writing);
-  std::optional<std::pair<std::string, bool>>
+  std::optional<std::pair<util::Blob, bool>>
   get_from_secondary_storage(const Digest& key);
 
   void put_in_secondary_storage(const Digest& key,
-                                const std::string& value,
+                                const util::Blob& value,
                                 bool only_if_missing);
 
   void remove_from_secondary_storage(const Digest& key);
diff --git a/src/storage/secondary/FileStorage.cpp b/src/storage/secondary/FileStorage.cpp
index b03dbf988..16580a6a1 100644
--- a/src/storage/secondary/FileStorage.cpp
+++ b/src/storage/secondary/FileStorage.cpp
@@ -43,11 +43,11 @@ class FileStorageBackend : public SecondaryStorage::Backend
 public:
   FileStorageBackend(const Params& params);
 
-  nonstd::expected<std::optional<std::string>, Failure>
+  nonstd::expected<std::optional<util::Blob>, Failure>
   get(const Digest& key) override;
 
   nonstd::expected<bool, Failure> put(const Digest& key,
-                                      const std::string& value,
+                                      const util::Blob& value,
                                       bool only_if_missing) override;
 
   nonstd::expected<bool, Failure> remove(const Digest& key) override;
@@ -103,7 +103,7 @@ FileStorageBackend::FileStorageBackend(const Params& params)
   }
 }
 
-nonstd::expected<std::optional<std::string>, SecondaryStorage::Backend::Failure>
+nonstd::expected<std::optional<util::Blob>, SecondaryStorage::Backend::Failure>
 FileStorageBackend::get(const Digest& key)
 {
   const auto path = get_entry_path(key);
@@ -120,18 +120,17 @@ FileStorageBackend::get(const Digest& key)
     util::set_timestamps(path);
   }
 
-  try {
-    LOG("Reading {}", path);
-    return Util::read_file(path);
-  } catch (const core::Error& e) {
-    LOG("Failed to read {}: {}", path, e.what());
+  auto value = util::read_file<util::Blob>(path);
+  if (!value) {
+    LOG("Failed to read {}: {}", path, value.error());
     return nonstd::make_unexpected(Failure::error);
   }
+  return std::move(*value);
 }
 
 nonstd::expected<bool, SecondaryStorage::Backend::Failure>
 FileStorageBackend::put(const Digest& key,
-                        const std::string& value,
+                        const util::Blob& value,
                         const bool only_if_missing)
 {
   const auto path = get_entry_path(key);
diff --git a/src/storage/secondary/HttpStorage.cpp b/src/storage/secondary/HttpStorage.cpp
index b73fe356d..5fed19d0c 100644
--- a/src/storage/secondary/HttpStorage.cpp
+++ b/src/storage/secondary/HttpStorage.cpp
@@ -25,6 +25,7 @@
 #include <fmtmacros.hpp>
 #include <util/expected.hpp>
 #include <util/string.hpp>
+#include <util/types.hpp>
 
 #include <third_party/httplib.h>
 #include <third_party/url.hpp>
@@ -40,11 +41,11 @@ class HttpStorageBackend : public SecondaryStorage::Backend
 public:
   HttpStorageBackend(const Params& params);
 
-  nonstd::expected<std::optional<std::string>, Failure>
+  nonstd::expected<std::optional<util::Blob>, Failure>
   get(const Digest& key) override;
 
   nonstd::expected<bool, Failure> put(const Digest& key,
-                                      const std::string& value,
+                                      const util::Blob& value,
                                       bool only_if_missing) override;
 
   nonstd::expected<bool, Failure> remove(const Digest& key) override;
@@ -143,7 +144,7 @@ HttpStorageBackend::HttpStorageBackend(const Params& params)
   m_http_client.set_write_timeout(operation_timeout);
 }
 
-nonstd::expected<std::optional<std::string>, SecondaryStorage::Backend::Failure>
+nonstd::expected<std::optional<util::Blob>, SecondaryStorage::Backend::Failure>
 HttpStorageBackend::get(const Digest& key)
 {
   const auto url_path = get_entry_path(key);
@@ -162,12 +163,12 @@ HttpStorageBackend::get(const Digest& key)
     return std::nullopt;
   }
 
-  return result->body;
+  return util::Blob(result->body.begin(), result->body.end());
 }
 
 nonstd::expected<bool, SecondaryStorage::Backend::Failure>
 HttpStorageBackend::put(const Digest& key,
-                        const std::string& value,
+                        const util::Blob& value,
                         const bool only_if_missing)
 {
   const auto url_path = get_entry_path(key);
@@ -193,7 +194,10 @@ HttpStorageBackend::put(const Digest& key,
 
   static const auto content_type = "application/octet-stream";
   const auto result =
-    m_http_client.Put(url_path, value.data(), value.size(), content_type);
+    m_http_client.Put(url_path,
+                      reinterpret_cast<const char*>(value.data()),
+                      value.size(),
+                      content_type);
 
   if (result.error() != httplib::Error::Success || !result) {
     LOG("Failed to put {} to http storage: {} ({})",
diff --git a/src/storage/secondary/RedisStorage.cpp b/src/storage/secondary/RedisStorage.cpp
index 68bac59e4..cb985a688 100644
--- a/src/storage/secondary/RedisStorage.cpp
+++ b/src/storage/secondary/RedisStorage.cpp
@@ -60,11 +60,11 @@ class RedisStorageBackend : public SecondaryStorage::Backend
 public:
   RedisStorageBackend(const SecondaryStorage::Backend::Params& params);
 
-  nonstd::expected<std::optional<std::string>, Failure>
+  nonstd::expected<std::optional<util::Blob>, Failure>
   get(const Digest& key) override;
 
   nonstd::expected<bool, Failure> put(const Digest& key,
-                                      const std::string& value,
+                                      const util::Blob& value,
                                       bool only_if_missing) override;
 
   nonstd::expected<bool, Failure> remove(const Digest& key) override;
@@ -157,7 +157,7 @@ is_timeout(int err)
 #endif
 }
 
-nonstd::expected<std::optional<std::string>, SecondaryStorage::Backend::Failure>
+nonstd::expected<std::optional<util::Blob>, SecondaryStorage::Backend::Failure>
 RedisStorageBackend::get(const Digest& key)
 {
   const auto key_string = get_key_string(key);
@@ -166,7 +166,7 @@ RedisStorageBackend::get(const Digest& key)
   if (!reply) {
     return nonstd::make_unexpected(reply.error());
   } else if ((*reply)->type == REDIS_REPLY_STRING) {
-    return std::string((*reply)->str, (*reply)->len);
+    return util::Blob((*reply)->str, (*reply)->str + (*reply)->len);
   } else if ((*reply)->type == REDIS_REPLY_NIL) {
     return std::nullopt;
   } else {
@@ -177,7 +177,7 @@ RedisStorageBackend::get(const Digest& key)
 
 nonstd::expected<bool, SecondaryStorage::Backend::Failure>
 RedisStorageBackend::put(const Digest& key,
-                         const std::string& value,
+                         const util::Blob& value,
                          bool only_if_missing)
 {
   const auto key_string = get_key_string(key);
diff --git a/src/storage/secondary/SecondaryStorage.hpp b/src/storage/secondary/SecondaryStorage.hpp
index 64eaf7aed..72911418a 100644
--- a/src/storage/secondary/SecondaryStorage.hpp
+++ b/src/storage/secondary/SecondaryStorage.hpp
@@ -19,6 +19,7 @@
 #pragma once
 
 #include <storage/types.hpp>
+#include <util/types.hpp>
 
 #include <third_party/nonstd/expected.hpp>
 #include <third_party/url.hpp>
@@ -78,7 +79,7 @@ public:
 
     // Get the value associated with `key`. Returns the value on success or
     // std::nullopt if the entry is not present.
-    virtual nonstd::expected<std::optional<std::string>, Failure>
+    virtual nonstd::expected<std::optional<util::Blob>, Failure>
     get(const Digest& key) = 0;
 
     // Put `value` associated to `key` in the storage. A true `only_if_missing`
@@ -86,7 +87,7 @@ public:
     // Returns true if the entry was stored, otherwise false.
     virtual nonstd::expected<bool, Failure>
     put(const Digest& key,
-        const std::string& value,
+        const util::Blob& value,
         bool only_if_missing = false) = 0;
 
     // Remove `key` and its associated value. Returns true if the entry was