refactor(tvix/glue): return a parsed Url in NixFetchArgs r/8307

The only two consumers (fetchurl, fetchtarball) of these do try to parse
it as URL, so do it in the helper.

Update url_basename to take a &url::URL, not a &str.

Also update the test to use rstest for the fixtures to reduce some
boilerplate there.

Change-Id: I1f85fe2803060dc4423e673cb7b9f9bf799d09b9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11875
Reviewed-by: Ilan Joselevich <personal@ilanjoselevich.com>
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>

2 files changed, 29 insertions, 44 deletions

diff --git a/tvix/glue/src/builtins/fetchers.rs b/tvix/glue/src/builtins/fetchers.rs
index d95287aeca3a..47da71fb922b 100644
--- a/tvix/glue/src/builtins/fetchers.rs
+++ b/tvix/glue/src/builtins/fetchers.rs
@@ -11,9 +11,12 @@ use tvix_eval::builtin_macros::builtins;
 use tvix_eval::generators::Gen;
 use tvix_eval::generators::GenCo;
 use tvix_eval::{CatchableErrorKind, ErrorKind, Value};
+use url::Url;
 
+// Used as a return type for extract_fetch_args, which is sharing some
+// parsing code between the fetchurl and fetchTarball builtins.
 struct NixFetchArgs {
-    url_str: String,
+    url: Url,
     name: Option<String>,
     sha256: Option<[u8; 32]>,
 }
@@ -28,8 +31,12 @@ async fn extract_fetch_args(
         // Get the raw bytes, not the ToString repr.
         let url_str =
             String::from_utf8(url_str.as_bytes().to_vec()).map_err(|_| ErrorKind::Utf8)?;
+
+        // Parse the URL.
+        let url = Url::parse(&url_str).map_err(|e| ErrorKind::TvixError(Rc::new(e)))?;
+
         return Ok(Ok(NixFetchArgs {
-            url_str,
+            url,
             name: None,
             sha256: None,
         }));
@@ -67,19 +74,16 @@ async fn extract_fetch_args(
         None => None,
     };
 
-    Ok(Ok(NixFetchArgs {
-        url_str,
-        name,
-        sha256,
-    }))
+    // Parse the URL.
+    let url = Url::parse(&url_str).map_err(|e| ErrorKind::TvixError(Rc::new(e)))?;
+
+    Ok(Ok(NixFetchArgs { url, name, sha256 }))
 }
 
 #[allow(unused_variables)] // for the `state` arg, for now
 #[builtins(state = "Rc<TvixStoreIO>")]
 pub(crate) mod fetcher_builtins {
-    use crate::builtins::FetcherError;
     use nix_compat::nixhash::NixHash;
-    use url::Url;
 
     use super::*;
 
@@ -135,17 +139,13 @@ pub(crate) mod fetcher_builtins {
         // Derive the name from the URL basename if not set explicitly.
         let name = args
             .name
-            .unwrap_or_else(|| url_basename(&args.url_str).to_owned());
-
-        // Parse the URL.
-        let url = Url::parse(&args.url_str)
-            .map_err(|e| ErrorKind::TvixError(Rc::new(FetcherError::InvalidUrl(e))))?;
+            .unwrap_or_else(|| url_basename(&args.url).to_owned());
 
         fetch_lazy(
             state,
             name,
             Fetch::URL {
-                url,
+                url: args.url,
                 exp_hash: args.sha256.map(NixHash::Sha256),
             },
         )
@@ -168,15 +168,11 @@ pub(crate) mod fetcher_builtins {
             .name
             .unwrap_or_else(|| DEFAULT_NAME_FETCH_TARBALL.to_owned());
 
-        // Parse the URL.
-        let url = Url::parse(&args.url_str)
-            .map_err(|e| ErrorKind::TvixError(Rc::new(FetcherError::InvalidUrl(e))))?;
-
         fetch_lazy(
             state,
             name,
             Fetch::Tarball {
-                url,
+                url: args.url,
                 exp_nar_sha256: args.sha256,
             },
         )
diff --git a/tvix/glue/src/fetchers/mod.rs b/tvix/glue/src/fetchers/mod.rs
index 6cce569e9799..eb035a5a905c 100644
--- a/tvix/glue/src/fetchers/mod.rs
+++ b/tvix/glue/src/fetchers/mod.rs
@@ -603,7 +603,8 @@ where
 }
 
 /// Attempts to mimic `nix::libutil::baseNameOf`
-pub(crate) fn url_basename(s: &str) -> &str {
+pub(crate) fn url_basename(url: &Url) -> &str {
+    let s = url.path();
     if s.is_empty() {
         return "";
     }
@@ -720,30 +721,18 @@ mod tests {
 
     mod url_basename {
         use super::super::*;
+        use rstest::rstest;
 
-        #[test]
-        fn empty_path() {
-            assert_eq!(url_basename(""), "");
-        }
-
-        #[test]
-        fn path_on_root() {
-            assert_eq!(url_basename("/dir"), "dir");
-        }
-
-        #[test]
-        fn relative_path() {
-            assert_eq!(url_basename("dir/foo"), "foo");
-        }
-
-        #[test]
-        fn root_with_trailing_slash() {
-            assert_eq!(url_basename("/"), "");
-        }
-
-        #[test]
-        fn trailing_slash() {
-            assert_eq!(url_basename("/dir/"), "dir");
+        #[rstest]
+        #[case::empty_path("", "")]
+        #[case::path_on_root("/dir", "dir")]
+        #[case::relative_path("dir/foo", "foo")]
+        #[case::root_with_trailing_slash("/", "")]
+        #[case::trailing_slash("/dir/", "dir")]
+        fn test_url_basename(#[case] url_path: &str, #[case] exp_basename: &str) {
+            let mut url = Url::parse("http://localhost").expect("invalid url");
+            url.set_path(url_path);
+            assert_eq!(url_basename(&url), exp_basename);
         }
     }
 }