Merge pull request #13739 from obsidiansystems/getUri-not-string

Rewrite `StoreConfig::getUri` in terms of new `StoreConfig::getReference`

src/libstore-tests/http-binary-cache-store.cc

@@ -8,14 +8,14 @@ TEST(HttpBinaryCacheStore, constructConfig)
 {
     HttpBinaryCacheStoreConfig config{"http", "foo.bar.baz", {}};
 
-    EXPECT_EQ(config.cacheUri, "http://foo.bar.baz");
+    EXPECT_EQ(config.cacheUri.to_string(), "http://foo.bar.baz");
 }
 
 TEST(HttpBinaryCacheStore, constructConfigNoTrailingSlash)
 {
     HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b/", {}};
 
-    EXPECT_EQ(config.cacheUri, "https://foo.bar.baz/a/b");
+    EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b");
 }
 
 } // namespace nix

src/libstore-tests/nix_api_store.cc

@@ -24,7 +24,7 @@ TEST_F(nix_api_store_test, nix_store_get_uri)
     std::string str;
     auto ret = nix_store_get_uri(ctx, store, OBSERVE_STRING(str));
     ASSERT_EQ(NIX_OK, ret);
-    auto expectedStoreURI = "local?"
+    auto expectedStoreURI = "local://?"
                             + nix::encodeQuery({
                                 {"log", nixLogDir},
                                 {"state", nixStateDir},
@@ -104,7 +104,7 @@ TEST_F(nix_api_util_context, nix_store_open_dummy)
     nix_libstore_init(ctx);
     Store * store = nix_store_open(ctx, "dummy://", nullptr);
     ASSERT_EQ(NIX_OK, ctx->last_err_code);
-    ASSERT_STREQ("dummy", store->ptr->config.getUri().c_str());
+    ASSERT_STREQ("dummy://", store->ptr->config.getUri().c_str());
 
     std::string str;
     nix_store_get_version(ctx, store, OBSERVE_STRING(str));

src/libstore/dummy-store.cc

@@ -33,9 +33,14 @@ struct DummyStoreConfig : public std::enable_shared_from_this<DummyStoreConfig>,
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override
+    StoreReference getReference() const override
     {
-        return *uriSchemes().begin();
+        return {
+            .variant =
+                StoreReference::Specified{
+                    .scheme = *uriSchemes().begin(),
+                },
+        };
     }
 };
 

src/libstore/http-binary-cache-store.cc

@@ -22,13 +22,25 @@ HttpBinaryCacheStoreConfig::HttpBinaryCacheStoreConfig(
     std::string_view scheme, std::string_view _cacheUri, const Params & params)
     : StoreConfig(params)
     , BinaryCacheStoreConfig(params)
-    , cacheUri(
+    , cacheUri(parseURL(
           std::string{scheme} + "://"
           + (!_cacheUri.empty() ? _cacheUri
-                                : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme)))
+                                : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme))))
 {
-    while (!cacheUri.empty() && cacheUri.back() == '/')
+    while (!cacheUri.path.empty() && cacheUri.path.back() == '/')
-        cacheUri.pop_back();
+        cacheUri.path.pop_back();
+}
+
+StoreReference HttpBinaryCacheStoreConfig::getReference() const
+{
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = cacheUri.scheme,
+                .authority = (cacheUri.authority ? cacheUri.authority->to_string() : "") + cacheUri.path,
+            },
+        .params = cacheUri.query,
+    };
 }
 
 std::string HttpBinaryCacheStoreConfig::doc()
@@ -65,16 +77,17 @@ public:
     void init() override
     {
         // FIXME: do this lazily?
-        if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri)) {
+        if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri.to_string())) {
             config->wantMassQuery.setDefault(cacheInfo->wantMassQuery);
             config->priority.setDefault(cacheInfo->priority);
         } else {
             try {
                 BinaryCacheStore::init();
             } catch (UploadToHTTP &) {
-                throw Error("'%s' does not appear to be a binary cache", config->cacheUri);
+                throw Error("'%s' does not appear to be a binary cache", config->cacheUri.to_string());
             }
-            diskCache->createCache(config->cacheUri, config->storeDir, config->wantMassQuery, config->priority);
+            diskCache->createCache(
+                config->cacheUri.to_string(), config->storeDir, config->wantMassQuery, config->priority);
         }
     }
 
@@ -134,16 +147,29 @@ protected:
         try {
             getFileTransfer()->upload(req);
         } catch (FileTransferError & e) {
-            throw UploadToHTTP("while uploading to HTTP binary cache at '%s': %s", config->cacheUri, e.msg());
+            throw UploadToHTTP(
+                "while uploading to HTTP binary cache at '%s': %s", config->cacheUri.to_string(), e.msg());
         }
     }
 
     FileTransferRequest makeRequest(const std::string & path)
     {
+        /* FIXME path is not a path, but a full relative or absolute
+           URL, e.g. we've seen in the wild NARINFO files have a URL
+           field which is
+           `nar/15f99rdaf26k39knmzry4xd0d97wp6yfpnfk1z9avakis7ipb9yg.nar?hash=zphkqn2wg8mnvbkixnl2aadkbn0rcnfj`
+           (note the query param) and that gets passed here.
+
+           What should actually happen is that we have two parsed URLs
+           (if we support relative URLs), and then we combined them with
+           a URL `operator/` which would be like
+           `std::filesystem::path`'s equivalent operator, which properly
+           combines the the URLs, whether the right is relative or
+           absolute. */
         return FileTransferRequest(
             hasPrefix(path, "https://") || hasPrefix(path, "http://") || hasPrefix(path, "file://")
                 ? path
-                : config->cacheUri + "/" + path);
+                : config->cacheUri.to_string() + "/" + path);
     }
 
     void getFile(const std::string & path, Sink & sink) override

src/libstore/include/nix/store/http-binary-cache-store.hh

@@ -1,3 +1,4 @@
+#include "nix/util/url.hh"
 #include "nix/store/binary-cache-store.hh"
 
 namespace nix {
@@ -11,7 +12,7 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this<HttpBinaryCache
     HttpBinaryCacheStoreConfig(
         std::string_view scheme, std::string_view cacheUri, const Store::Config::Params & params);
 
-    Path cacheUri;
+    ParsedURL cacheUri;
 
     static const std::string name()
     {
@@ -24,10 +25,7 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this<HttpBinaryCache
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override
+    StoreReference getReference() const override;
-    {
-        return cacheUri;
-    }
 };
 
 } // namespace nix

src/libstore/include/nix/store/legacy-ssh-store.hh

@@ -54,7 +54,7 @@ struct LegacySSHStoreConfig : std::enable_shared_from_this<LegacySSHStoreConfig>
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 struct LegacySSHStore : public virtual Store

src/libstore/include/nix/store/local-binary-cache-store.hh

@@ -27,7 +27,7 @@ struct LocalBinaryCacheStoreConfig : std::enable_shared_from_this<LocalBinaryCac
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 } // namespace nix

src/libstore/include/nix/store/local-overlay-store.hh

@@ -88,10 +88,7 @@ struct LocalOverlayStoreConfig : virtual LocalStoreConfig
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override
+    StoreReference getReference() const override;
-    {
-        return "local-overlay://";
-    }
 
 protected:
     /**

src/libstore/include/nix/store/local-store.hh

@@ -112,7 +112,7 @@ struct LocalStoreConfig : std::enable_shared_from_this<LocalStoreConfig>,
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 class LocalStore : public virtual IndirectRootStore, public virtual GcStore

src/libstore/include/nix/store/s3-binary-cache-store.hh

@@ -107,7 +107,7 @@ public:
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 struct S3BinaryCacheStore : virtual BinaryCacheStore

src/libstore/include/nix/store/ssh-store.hh

@@ -34,7 +34,7 @@ struct SSHStoreConfig : std::enable_shared_from_this<SSHStoreConfig>,
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 struct MountedSSHStoreConfig : virtual SSHStoreConfig, virtual LocalFSStoreConfig

src/libstore/include/nix/store/store-api.hh

@@ -199,11 +199,16 @@ struct StoreConfig : public StoreDirConfig
     virtual ref<Store> openStore() const = 0;
 
     /**
-     * Render the config back to a "store URL". It should round-trip
+     * Render the config back to a `StoreReference`. It should round-trip
      * with `resolveStoreConfig` (for stores configs that are
      * registered).
      */
-    virtual std::string getUri() const;
+    virtual StoreReference getReference() const;
+
+    std::string getUri() const
+    {
+        return getReference().render();
+    }
 };
 
 /**

src/libstore/include/nix/store/uds-remote-store.hh

@@ -45,7 +45,7 @@ struct UDSRemoteStoreConfig : std::enable_shared_from_this<UDSRemoteStoreConfig>
 
     ref<Store> openStore() const override;
 
-    std::string getUri() const override;
+    StoreReference getReference() const override;
 };
 
 struct UDSRemoteStore : virtual IndirectRootStore, virtual RemoteStore

src/libstore/legacy-ssh-store.cc

@@ -88,9 +88,16 @@ ref<LegacySSHStore::Connection> LegacySSHStore::openConnection()
     return conn;
 };
 
-std::string LegacySSHStoreConfig::getUri() const
+StoreReference LegacySSHStoreConfig::getReference() const
 {
-    return ParsedURL{.scheme = *uriSchemes().begin(), .authority = authority, .query = getQueryParams()}.to_string();
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+                .authority = authority.to_string(),
+            },
+        .params = getQueryParams(),
+    };
 }
 
 std::map<StorePath, UnkeyedValidPathInfo> LegacySSHStore::queryPathInfosUncached(const StorePathSet & paths)

src/libstore/local-binary-cache-store.cc

@@ -23,9 +23,15 @@ std::string LocalBinaryCacheStoreConfig::doc()
         ;
 }
 
-std::string LocalBinaryCacheStoreConfig::getUri() const
+StoreReference LocalBinaryCacheStoreConfig::getReference() const
 {
-    return "file://" + binaryCacheDir;
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = "file",
+                .authority = binaryCacheDir,
+            },
+    };
 }
 
 struct LocalBinaryCacheStore : virtual BinaryCacheStore

src/libstore/local-overlay-store.cc

@@ -23,6 +23,16 @@ ref<Store> LocalOverlayStoreConfig::openStore() const
         ref{std::dynamic_pointer_cast<const LocalOverlayStoreConfig>(shared_from_this())});
 }
 
+StoreReference LocalOverlayStoreConfig::getReference() const
+{
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+            },
+    };
+}
+
 Path LocalOverlayStoreConfig::toUpperPath(const StorePath & path) const
 {
     return upperLayer + "/" + path.to_string();

src/libstore/local-store.cc

@@ -439,15 +439,15 @@ LocalStore::~LocalStore()
     }
 }
 
-std::string LocalStoreConfig::getUri() const
+StoreReference LocalStoreConfig::getReference() const
 {
-    std::ostringstream oss;
+    return {
-    oss << *uriSchemes().begin();
+        .variant =
-    auto queryParams = getQueryParams();
+            StoreReference::Specified{
-    if (!queryParams.empty())
+                .scheme = *uriSchemes().begin(),
-        oss << "?";
+            },
-    oss << encodeQuery(queryParams);
+        .params = getQueryParams(),
-    return std::move(oss).str();
+    };
 }
 
 int LocalStore::getSchema()

src/libstore/s3-binary-cache-store.cc

@@ -254,9 +254,15 @@ std::string S3BinaryCacheStoreConfig::doc()
         ;
 }
 
-std::string S3BinaryCacheStoreConfig::getUri() const
+StoreReference S3BinaryCacheStoreConfig::getReference() const
 {
-    return "s3://" + bucketName;
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+                .authority = bucketName,
+            },
+    };
 }
 
 struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStore

src/libstore/ssh-store.cc

@@ -25,9 +25,16 @@ std::string SSHStoreConfig::doc()
         ;
 }
 
-std::string SSHStoreConfig::getUri() const
+StoreReference SSHStoreConfig::getReference() const
 {
-    return ParsedURL{.scheme = *uriSchemes().begin(), .authority = authority, .query = getQueryParams()}.to_string();
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+                .authority = authority.to_string(),
+            },
+        .params = getQueryParams(),
+    };
 }
 
 struct SSHStore : virtual RemoteStore

src/libstore/store-api.cc

@@ -300,9 +300,9 @@ Store::Store(const Store::Config & config)
     assertLibStoreInitialized();
 }
 
-std::string StoreConfig::getUri() const
+StoreReference StoreConfig::getReference() const
 {
-    return "";
+    return {.variant = StoreReference::Auto{}};
 }
 
 bool Store::PathInfoCacheValue::isKnownNow()

src/libstore/uds-remote-store.cc

@@ -54,14 +54,19 @@ UDSRemoteStore::UDSRemoteStore(ref<const Config> config)
 {
 }
 
-std::string UDSRemoteStoreConfig::getUri() const
+StoreReference UDSRemoteStoreConfig::getReference() const
 {
-    return path == settings.nixDaemonSocketFile ? // FIXME: Not clear why we return daemon here and not default
+    return {
-                                                  // to settings.nixDaemonSocketFile
+        .variant =
-                                                  //
+            StoreReference::Specified{
-                                                  // unix:// with no path also works. Change what we return?
+                .scheme = *uriSchemes().begin(),
-               "daemon"
+                // We return the empty string when the path looks like the
-                                                : std::string(*uriSchemes().begin()) + "://" + path;
+                // default path, but we could also just return the path
+                // verbatim always, to be robust to overall config changes
+                // at the cost of some verbosity.
+                .authority = path == settings.nixDaemonSocketFile ? "" : path,
+            },
+    };
 }
 
 void UDSRemoteStore::Connection::closeWrite()

tests/functional/store-info.sh

@@ -2,12 +2,61 @@
 
 source common.sh
 
+# Different versions of the Nix daemon normalize or don't normalize
+# store URLs, plus NIX_REMOTE (per the test suite) might not be using on
+# store URL in normal form, so the easiest thing to do is normalize URLs
+# after the fact before comparing them for equality.
+normalize_nix_store_url () {
+    local url="$1"
+    case "$url" in
+        'auto' )
+            # Need to actually ask Nix in this case
+            echo "$defaultStore"
+            ;;
+        'local://'* )
+            # To not be captured by next pattern
+            echo "$url"
+            ;;
+        local | 'local?'* )
+            echo "local://${url#local}"
+            ;;
+        daemon | 'daemon?'* )
+            echo "unix://${url#daemon}"
+            ;;
+        * )
+            echo "$url"
+            ;;
+    esac
+}
+
 STORE_INFO=$(nix store info 2>&1)
 LEGACY_STORE_INFO=$(nix store ping 2>&1) # alias to nix store info
 STORE_INFO_JSON=$(nix store info --json)
 
-echo "$STORE_INFO" | grep "Store URL: ${NIX_REMOTE}"
+defaultStore="$(normalize_nix_store_url "$(echo "$STORE_INFO_JSON" | jq -r ".url")")"
-echo "$LEGACY_STORE_INFO" | grep "Store URL: ${NIX_REMOTE}"
+
+# Test cases for `normalize_nix_store_url` itself
+
+# Normalize local store
+[[ "$(normalize_nix_store_url "local://")" = "local://" ]]
+[[ "$(normalize_nix_store_url "local")" = "local://" ]]
+[[ "$(normalize_nix_store_url "local?foo=bar")" = "local://?foo=bar" ]]
+
+# Normalize unix domain socket remote store
+[[ "$(normalize_nix_store_url "unix://")" = "unix://" ]]
+[[ "$(normalize_nix_store_url "daemon")" = "unix://" ]]
+[[ "$(normalize_nix_store_url "daemon?x=y")" = "unix://?x=y" ]]
+
+# otherwise unchanged
+[[ "$(normalize_nix_store_url "https://site")" = "https://site" ]]
+
+nixRemoteOrDefault=$(normalize_nix_store_url "${NIX_REMOTE:-"auto"}")
+
+check_human_readable () {
+    [[ "$(normalize_nix_store_url "$(echo "$1" | grep 'Store URL:' | sed 's^Store URL: ^^')")" = "${nixRemoteOrDefault}" ]]
+}
+check_human_readable "$STORE_INFO"
+check_human_readable "$LEGACY_STORE_INFO"
 
 if [[ -v NIX_DAEMON_PACKAGE ]] && isDaemonNewer "2.7.0pre20220126"; then
     DAEMON_VERSION=$("$NIX_DAEMON_PACKAGE"/bin/nix daemon --version | cut -d' ' -f3)
@@ -21,4 +70,4 @@ expect 127 NIX_REMOTE=unix:"$PWD"/store nix store info || \
 
 TODO_NixOS
 
-[[ "$(echo "$STORE_INFO_JSON" | jq -r ".url")" == "${NIX_REMOTE:-local}" ]]
+[[ "$(normalize_nix_store_url "$(echo "$STORE_INFO_JSON" | jq -r ".url")")" == "${nixRemoteOrDefault}" ]]