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

xokdvium · web-flow · commit cf7084a67c8c · 2025-08-14T02:46:48.000+03:00
Rewrite `StoreConfig::getUri` in terms of new `StoreConfig::getReference`
diff --git a/src/libstore-tests/http-binary-cache-store.cc b/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
diff --git a/src/libstore-tests/nix_api_store.cc b/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));
diff --git a/src/libstore/dummy-store.cc b/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(),
+                },
+        };
     }
 };
 
diff --git a/src/libstore/http-binary-cache-store.cc b/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() == '/')
-        cacheUri.pop_back();
+    while (!cacheUri.path.empty() && cacheUri.path.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 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore
     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 @@ class HttpBinaryCacheStore : public virtual BinaryCacheStore
         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
diff --git a/src/libstore/include/nix/store/http-binary-cache-store.hh b/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
-    {
-        return cacheUri;
-    }
+    StoreReference getReference() const override;
 };
 
 } // namespace nix
diff --git a/src/libstore/include/nix/store/legacy-ssh-store.hh b/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
diff --git a/src/libstore/include/nix/store/local-binary-cache-store.hh b/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
diff --git a/src/libstore/include/nix/store/local-overlay-store.hh b/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
-    {
-        return "local-overlay://";
-    }
+    StoreReference getReference() const override;
 
 protected:
     /**
diff --git a/src/libstore/include/nix/store/local-store.hh b/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
diff --git a/src/libstore/include/nix/store/s3-binary-cache-store.hh b/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
diff --git a/src/libstore/include/nix/store/ssh-store.hh b/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
diff --git a/src/libstore/include/nix/store/store-api.hh b/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();
+    }
 };
 
 /**
diff --git a/src/libstore/include/nix/store/uds-remote-store.hh b/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
diff --git a/src/libstore/legacy-ssh-store.cc b/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)
diff --git a/src/libstore/local-binary-cache-store.cc b/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
diff --git a/src/libstore/local-overlay-store.cc b/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();
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
@@ -439,15 +439,15 @@ LocalStore::~LocalStore()
     }
 }
 
-std::string LocalStoreConfig::getUri() const
+StoreReference LocalStoreConfig::getReference() const
 {
-    std::ostringstream oss;
-    oss << *uriSchemes().begin();
-    auto queryParams = getQueryParams();
-    if (!queryParams.empty())
-        oss << "?";
-    oss << encodeQuery(queryParams);
-    return std::move(oss).str();
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+            },
+        .params = getQueryParams(),
+    };
 }
 
 int LocalStore::getSchema()
diff --git a/src/libstore/s3-binary-cache-store.cc b/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
diff --git a/src/libstore/ssh-store.cc b/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
diff --git a/src/libstore/store-api.cc b/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()
diff --git a/src/libstore/uds-remote-store.cc b/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
-                                                  // to settings.nixDaemonSocketFile
-                                                  //
-                                                  // unix:// with no path also works. Change what we return?
-               "daemon"
-                                                : std::string(*uriSchemes().begin()) + "://" + path;
+    return {
+        .variant =
+            StoreReference::Specified{
+                .scheme = *uriSchemes().begin(),
+                // We return the empty string when the path looks like the
+                // 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()
diff --git a/tests/functional/store-info.sh b/tests/functional/store-info.sh

Original file line number	Diff line number	Diff line change
`@@ -8,14 +8,14 @@ TEST(HttpBinaryCacheStore, constructConfig)`
`8`	`8`	`{`
`9`	`9`	`HttpBinaryCacheStoreConfig config{"http", "foo.bar.baz", {}};`
`10`	`10`
`11`		`- EXPECT_EQ(config.cacheUri, "http://foo.bar.baz");`
	`11`	`+ EXPECT_EQ(config.cacheUri.to_string(), "http://foo.bar.baz");`
`12`	`12`	`}`
`13`	`13`
`14`	`14`	`TEST(HttpBinaryCacheStore, constructConfigNoTrailingSlash)`
`15`	`15`	`{`
`16`	`16`	`HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b/", {}};`
`17`	`17`
`18`		`- EXPECT_EQ(config.cacheUri, "https://foo.bar.baz/a/b");`
	`18`	`+ EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b");`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`} // namespace nix`
Original file line number	Diff line number	Diff line change
`@@ -33,9 +33,14 @@ struct DummyStoreConfig : public std::enable_shared_from_this<DummyStoreConfig>,`
`33`	`33`
`34`	`34`	`ref<Store> openStore() const override;`
`35`	`35`
`36`		`- std::string getUri() const override`
	`36`	`+ StoreReference getReference() const override`
`37`	`37`	`{`
`38`		`- return *uriSchemes().begin();`
	`38`	`+ return {`
	`39`	`+ .variant =`
	`40`	`+ StoreReference::Specified{`
	`41`	`+ .scheme = *uriSchemes().begin(),`
	`42`	`+ },`
	`43`	`+ };`
`39`	`44`	`}`
`40`	`45`	`};`
`41`	`46`