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>

1 files changed, 16 insertions, 20 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,
             },
         )