Merge pull request #12157 from DeterminateSystems/fix-path-flakeref-q…

…uery-without-fragment parsePathFlakeRefWithFragment(): Handle 'path?query' without a fragment
NixOS · Jan 9, 2025 · 2d9b213 · 2d9b213
2 parents 9b9e416 + 3ad0f45
commit 2d9b213
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 28 deletions.
diff --git a/src/libflake-tests/flakeref.cc b/src/libflake-tests/flakeref.cc
@@ -7,18 +7,60 @@ namespace nix {
 
 /* ----------- tests for flake/flakeref.hh --------------------------------------------------*/
 
-    /* ----------------------------------------------------------------------------
-     * to_string
-     * --------------------------------------------------------------------------*/
+    TEST(parseFlakeRef, path) {
+        experimentalFeatureSettings.experimentalFeatures.get().insert(Xp::Flakes);
+
+        fetchers::Settings fetchSettings;
+
+        {
+            auto s = "/foo/bar";
+            auto flakeref = parseFlakeRef(fetchSettings, s);
+            ASSERT_EQ(flakeref.to_string(), "path:/foo/bar");
+        }
+
+        {
+            auto s = "/foo/bar?revCount=123&rev=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+            auto flakeref = parseFlakeRef(fetchSettings, s);
+            ASSERT_EQ(flakeref.to_string(), "path:/foo/bar?rev=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&revCount=123");
+        }
+
+        {
+            auto s = "/foo/bar?xyzzy=123";
+            EXPECT_THROW(
+                parseFlakeRef(fetchSettings, s),
+                Error);
+        }
+
+        {
+            auto s = "/foo/bar#bla";
+            EXPECT_THROW(
+                parseFlakeRef(fetchSettings, s),
+                Error);
+        }
+
+        {
+            auto s = "/foo/bar#bla";
+            auto [flakeref, fragment] = parseFlakeRefWithFragment(fetchSettings, s);
+            ASSERT_EQ(flakeref.to_string(), "path:/foo/bar");
+            ASSERT_EQ(fragment, "bla");
+        }
+
+        {
+            auto s = "/foo/bar?revCount=123#bla";
+            auto [flakeref, fragment] = parseFlakeRefWithFragment(fetchSettings, s);
+            ASSERT_EQ(flakeref.to_string(), "path:/foo/bar?revCount=123");
+            ASSERT_EQ(fragment, "bla");
+        }
+    }
 
     TEST(to_string, doesntReencodeUrl) {
         fetchers::Settings fetchSettings;
         auto s = "http://localhost:8181/test/+3d.tar.gz";
         auto flakeref = parseFlakeRef(fetchSettings, s);
-        auto parsed = flakeref.to_string();
+        auto unparsed = flakeref.to_string();
         auto expected = "http://localhost:8181/test/%2B3d.tar.gz";
 
-        ASSERT_EQ(parsed, expected);
+        ASSERT_EQ(unparsed, expected);
     }
 
 }
diff --git a/src/libflake/flake/flakeref.cc b/src/libflake/flake/flakeref.cc
@@ -89,23 +89,16 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
     bool allowMissing,
     bool isFlake)
 {
-    std::string path = url;
-    std::string fragment = "";
-    std::map<std::string, std::string> query;
-    auto pathEnd = url.find_first_of("#?");
-    auto fragmentStart = pathEnd;
-    if (pathEnd != std::string::npos && url[pathEnd] == '?') {
-        fragmentStart = url.find("#");
-    }
-    if (pathEnd != std::string::npos) {
-        path = url.substr(0, pathEnd);
-    }
-    if (fragmentStart != std::string::npos) {
-        fragment = percentDecode(url.substr(fragmentStart+1));
-    }
-    if (pathEnd != std::string::npos && fragmentStart != std::string::npos && url[pathEnd] == '?') {
-        query = decodeQuery(url.substr(pathEnd + 1, fragmentStart - pathEnd - 1));
-    }
+    static std::regex pathFlakeRegex(
+        R"(([^?#]*)(\?([^#]*))?(#(.*))?)",
+        std::regex::ECMAScript);
+
+    std::smatch match;
+    auto succeeds = std::regex_match(url, match, pathFlakeRegex);
+    assert(succeeds);
+    auto path = match[1].str();
+    auto query = decodeQuery(match[3]);
+    auto fragment = percentDecode(match[5].str());
 
     if (baseDir) {
         /* Check if 'url' is a path (either absolute or relative
@@ -190,11 +183,13 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
         path = canonPath(path + "/" + getOr(query, "dir", ""));
     }
 
-    fetchers::Attrs attrs;
-    attrs.insert_or_assign("type", "path");
-    attrs.insert_or_assign("path", path);
-
-    return std::make_pair(FlakeRef(fetchers::Input::fromAttrs(fetchSettings, std::move(attrs)), ""), fragment);
+    return fromParsedURL(fetchSettings, {
+        .scheme = "path",
+        .authority = "",
+        .path = path,
+        .query = query,
+        .fragment = fragment
+    }, isFlake);
 }
 
 /**

diff --git a/src/libutil/config.hh b/src/libutil/config.hh
@@ -262,6 +262,7 @@ public:
     operator const T &() const { return value; }
     operator T &() { return value; }
     const T & get() const { return value; }
+    T & get() { return value; }
     template<typename U>
     bool operator ==(const U & v2) const { return value == v2; }
     template<typename U>

diff --git a/src/libutil/url.cc b/src/libutil/url.cc
@@ -22,7 +22,6 @@ ParsedURL parseURL(const std::string & url)
     std::smatch match;
 
     if (std::regex_match(url, match, uriRegex)) {
-        auto & base = match[1];
         std::string scheme = match[2];
         auto authority = match[3].matched
             ? std::optional<std::string>(match[3]) : std::nullopt;

diff --git a/tests/functional/flakes/flake-in-submodule.sh b/tests/functional/flakes/flake-in-submodule.sh
@@ -63,3 +63,16 @@ flakeref=git+file://$rootRepo\?submodules=1\&dir=submodule
 echo '"foo"' > "$rootRepo"/submodule/sub.nix
 [[ $(nix eval --json "$flakeref#sub" ) = '"foo"' ]]
 [[ $(nix flake metadata --json "$flakeref" | jq -r .locked.rev) = null ]]
+
+# Test that `nix flake metadata` parses `submodule` correctly.
+cat > "$rootRepo"/flake.nix <<EOF
+{
+    outputs = { self }: {
+    };
+}
+EOF
+git -C "$rootRepo" add flake.nix
+git -C "$rootRepo" commit -m "Add flake.nix"
+
+storePath=$(nix flake metadata --json "$rootRepo?submodules=1" | jq -r .path)
+[[ -e "$storePath/submodule" ]]