Use URL class for URL parsing

diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp
index d4d5d953e84db2105ac5d17043c3111f77ad8fb0..a8c402386a3c4ea6aa6d538b199f49b4b2615757 100644 (file)
--- a/src/storage/Storage.cpp
+++ b/src/storage/Storage.cpp
@@ -28,7 +28,10 @@
 #include <util/Tokenizer.hpp>
 #include <util/string_utils.hpp>
 
+#include <third_party/url.hpp>
+
 #include <memory>
+#include <tuple>
 
 namespace storage {
 
@@ -179,8 +182,7 @@ namespace {
 
 struct ParseStorageEntryResult
 {
-  std::string scheme;
-  std::string url;
+  Url url;
   storage::AttributeMap attributes;
   bool read_only = false;
 };
@@ -190,16 +192,30 @@ struct ParseStorageEntryResult
 static ParseStorageEntryResult
 parse_storage_entry(const nonstd::string_view& entry)
 {
-  const auto parts = Util::split_into_views(entry, "|");
+  const auto parts =
+    Util::split_into_views(entry, "|", util::Tokenizer::Mode::include_empty);
+
+  if (parts.empty() || parts.front().empty()) {
+    throw Error("secondary storage config must provide a URL: {}", entry);
+  }
 
   ParseStorageEntryResult result;
-  result.url = parts.empty() ? "" : std::string(parts[0]);
-  const auto colon_slash_slash_pos = result.url.find("://");
-  if (colon_slash_slash_pos != std::string::npos) {
-    result.scheme = result.url.substr(0, colon_slash_slash_pos);
+  result.url = std::string(parts[0]);
+  // Url class is parsing the URL object lazily; check if successful
+  try {
+    std::ignore = result.url.host();
+  } catch (Url::parse_error& e) {
+    throw Error("Cannot parse URL: {}", e.what());
+  }
+
+  if (result.url.scheme().empty()) {
+    throw Error("URL scheme must not be empty: {}", entry);
   }
 
   for (size_t i = 1; i < parts.size(); ++i) {
+    if (parts[i].empty()) {
+      continue;
+    }
     const auto kv_pair = util::split_once(parts[i], '=');
     const auto& key = kv_pair.first;
     const auto& value = kv_pair.second ? *kv_pair.second : "true";
@@ -220,7 +236,7 @@ parse_storage_entry(const nonstd::string_view& entry)
 static std::unique_ptr<SecondaryStorage>
 create_storage(const ParseStorageEntryResult& storage_entry)
 {
-  if (storage_entry.scheme == "file") {
+  if (storage_entry.url.scheme() == "file") {
     return std::make_unique<secondary::FileStorage>(storage_entry.url,
                                                     storage_entry.attributes);
   }
@@ -234,11 +250,12 @@ Storage::add_secondary_storages()
   for (const auto& entry : util::Tokenizer(m_config.secondary_storage(), " ")) {
     const auto storage_entry = parse_storage_entry(entry);
     auto storage = create_storage(storage_entry);
+    auto url_for_logging = storage_entry.url.str();
     if (!storage) {
-      throw Error("unknown secondary storage URL: {}", storage_entry.url);
+      throw Error("unknown secondary storage URL: {}", url_for_logging);
     }
     m_secondary_storages.push_back(SecondaryStorageEntry{
-      std::move(storage), storage_entry.url, storage_entry.read_only});
+      std::move(storage), url_for_logging, storage_entry.read_only});
   }
 }
 

diff --git a/src/storage/Storage.hpp b/src/storage/Storage.hpp
index f15166aad05850b9b955dce3a8f5b8b20f1a2986..ca1969312bf33f2e906d9d6d3e64c3bf6ff45af6 100644 (file)
--- a/src/storage/Storage.hpp
+++ b/src/storage/Storage.hpp
@@ -59,7 +59,7 @@ private:
   struct SecondaryStorageEntry
   {
     std::unique_ptr<storage::SecondaryStorage> backend;
-    std::string url;
+    std::string url; // the Url class has a too-chatty stream operator
     bool read_only = false;
   };
 

diff --git a/src/storage/secondary/FileStorage.cpp b/src/storage/secondary/FileStorage.cpp
index 677dc3b974512beac59005e67b7ea76c3dbeaf85..3c0e5985bff440e46a1e1c95c061b395bf7f0e8e 100644 (file)
--- a/src/storage/secondary/FileStorage.cpp
+++ b/src/storage/secondary/FileStorage.cpp
@@ -34,14 +34,13 @@ namespace storage {
 namespace secondary {
 
 static std::string
-parse_url(const std::string& url)
+parse_url(const Url& url)
 {
-  const nonstd::string_view prefix = "file://";
-  ASSERT(Util::starts_with(url, prefix));
-  const auto dir = url.substr(prefix.size());
+  ASSERT(url.scheme() == "file");
+  const auto& dir = url.path();
   if (!Util::starts_with(dir, "/")) {
-    throw Error("invalid file URL \"{}\" - directory must start with a slash",
-                url);
+    throw Error("invalid file path \"{}\" - directory must start with a slash",
+                dir);
   }
   return dir;
 }
@@ -70,7 +69,7 @@ parse_update_mtime(const AttributeMap& attributes)
   return it != attributes.end() && it->second == "true";
 }
 
-FileStorage::FileStorage(const std::string& url, const AttributeMap& attributes)
+FileStorage::FileStorage(const Url& url, const AttributeMap& attributes)
   : m_dir(parse_url(url)),
     m_umask(parse_umask(attributes)),
     m_update_mtime(parse_update_mtime(attributes))

diff --git a/src/storage/secondary/FileStorage.hpp b/src/storage/secondary/FileStorage.hpp
index 18527b3ebdb37a77d17b769ff4a1c282139100d5..bcf9de1ed5d5b245df494c3a815f967cca1ff06b 100644 (file)
--- a/src/storage/secondary/FileStorage.hpp
+++ b/src/storage/secondary/FileStorage.hpp
@@ -21,13 +21,15 @@
 #include <storage/SecondaryStorage.hpp>
 #include <storage/types.hpp>
 
+#include <third_party/url.hpp>
+
 namespace storage {
 namespace secondary {
 
 class FileStorage : public storage::SecondaryStorage
 {
 public:
-  FileStorage(const std::string& url, const AttributeMap& attributes);
+  FileStorage(const Url& url, const AttributeMap& attributes);
 
   nonstd::expected<nonstd::optional<std::string>, Error>
   get(const Digest& key) override;

diff --git a/src/third_party/CMakeLists.txt b/src/third_party/CMakeLists.txt
index f8a041e267fcdfc945380e50d38cbe98e353bb33..3a06ee276240d0889e03d02a95d2e050c043f23c 100644 (file)
--- a/src/third_party/CMakeLists.txt
+++ b/src/third_party/CMakeLists.txt
@@ -1,4 +1,4 @@
-add_library(third_party_lib STATIC base32hex.c format.cpp xxhash.c url.cpp)
+add_library(third_party_lib STATIC base32hex.c format.cpp url.cpp xxhash.c)
 if(NOT MSVC)
   target_sources(third_party_lib PRIVATE getopt_long.c)
 else()

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 3dd1a36bd187199d59f5d834baa603162706644d..6353319a748b574bc7e4e806488a15474fe43787 100644 (file)
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -66,6 +66,7 @@ addtest(readonly)
 addtest(readonly_direct)
 addtest(sanitize_blacklist)
 addtest(secondary_file)
+addtest(secondary_url)
 addtest(serialize_diagnostics)
 addtest(source_date_epoch)
 addtest(split_dwarf)

diff --git a/test/suites/secondary_url.bash b/test/suites/secondary_url.bash
new file mode 100644 (file)
index 0000000..74fe082
--- /dev/null
+++ b/test/suites/secondary_url.bash
@@ -0,0 +1,33 @@
+SUITE_secondary_url_SETUP() {
+    generate_code 1 test.c
+}
+
+SUITE_secondary_url() {
+    # -------------------------------------------------------------------------
+    TEST "Reject empty url (without config attributes)"
+
+    export CCACHE_SECONDARY_STORAGE="|"
+    $CCACHE_COMPILE -c test.c 2>stderr.log
+    expect_contains stderr.log "must provide a URL"
+
+    # -------------------------------------------------------------------------
+    TEST "Reject empty url (but with config attributes)"
+
+    export CCACHE_SECONDARY_STORAGE="|key=value"
+    $CCACHE_COMPILE -c test.c 2>stderr.log
+    expect_contains stderr.log "must provide a URL"
+
+    # -------------------------------------------------------------------------
+    TEST "Reject invalid url"
+
+    export CCACHE_SECONDARY_STORAGE="://qwerty"
+    $CCACHE_COMPILE -c test.c 2>stderr.log
+    expect_contains stderr.log "Cannot parse URL"
+
+    # -------------------------------------------------------------------------
+    TEST "Reject missing scheme"
+
+    export CCACHE_SECONDARY_STORAGE="/qwerty"
+    $CCACHE_COMPILE -c test.c 2>stderr.log
+    expect_contains stderr.log "URL scheme must not be empty"
+}