refactor(tvix/store/pathinfosvc): inline SledPathInfoSvc::from_url r/7018

Change-Id: I0d905228df086a422bb30322add7236ca41e807b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10026
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>

2 files changed, 81 insertions, 136 deletions

diff --git a/tvix/store/src/pathinfoservice/from_addr.rs b/tvix/store/src/pathinfoservice/from_addr.rs
index 6cace25deaec..97e0a0f36e24 100644
--- a/tvix/store/src/pathinfoservice/from_addr.rs
+++ b/tvix/store/src/pathinfoservice/from_addr.rs
@@ -38,11 +38,30 @@ pub fn from_addr(
         }
         Arc::new(MemoryPathInfoService::new(blob_service, directory_service))
     } else if url.scheme() == "sled" {
-        Arc::new(SledPathInfoService::from_url(
-            &url,
-            blob_service,
-            directory_service,
-        )?)
+        // sled doesn't support host, and a path can be provided (otherwise
+        // it'll live in memory only).
+        if url.has_host() {
+            return Err(Error::StorageError("no host allowed".to_string()));
+        }
+
+        if url.path() == "/" {
+            return Err(Error::StorageError(
+                "cowardly refusing to open / with sled".to_string(),
+            ));
+        }
+
+        // TODO: expose compression and other parameters as URL parameters?
+
+        if url.path().is_empty() {
+            return Ok(Arc::new(
+                SledPathInfoService::new_temporary(blob_service, directory_service)
+                    .map_err(|e| Error::StorageError(e.to_string()))?,
+            ));
+        }
+        return Ok(Arc::new(
+            SledPathInfoService::new(url.path().into(), blob_service, directory_service)
+                .map_err(|e| Error::StorageError(e.to_string()))?,
+        ));
     } else if url.scheme().starts_with("grpc+") {
         // schemes starting with grpc+ go to the GRPCPathInfoService.
         //   That's normally grpc+unix for unix sockets, and grpc+http(s) for the HTTP counterparts.
@@ -61,6 +80,7 @@ pub fn from_addr(
 
 #[cfg(test)]
 mod tests {
+    use tempfile::TempDir;
     use tvix_castore::utils::{gen_blob_service, gen_directory_service};
 
     use super::from_addr;
@@ -100,6 +120,62 @@ mod tests {
         assert!(from_addr("memory:///foo", gen_blob_service(), gen_directory_service()).is_err())
     }
 
+    /// This uses the correct scheme, and doesn't specify a path (temporary sled).
+    #[test]
+    fn sled_valid_temporary() {
+        assert!(from_addr("sled://", gen_blob_service(), gen_directory_service()).is_ok())
+    }
+
+    /// This uses the correct scheme, and sets a tempdir as location.
+    #[test]
+    fn sled_valid_scheme_path() {
+        let tmpdir = TempDir::new().unwrap();
+
+        let mut url = url::Url::parse("sled://").expect("must parse");
+        url.set_path(tmpdir.path().to_str().unwrap());
+
+        assert!(from_addr(
+            &url.to_string(),
+            gen_blob_service(),
+            gen_directory_service()
+        )
+        .is_ok())
+    }
+
+    /// This uses the correct scheme, and specifies / as path (which should fail
+    // for obvious reasons)
+    #[test]
+    fn sled_fail_invalid_path_root() {
+        assert!(from_addr("sled:///", gen_blob_service(), gen_directory_service()).is_err())
+    }
+
+    /// This sets a host, rather than a path, which should fail.
+    #[test]
+    fn sled_invalid_host() {
+        assert!(from_addr(
+            "sled://foo.example",
+            gen_blob_service(),
+            gen_directory_service()
+        )
+        .is_err())
+    }
+
+    /// This sets a host AND a valid path, which should fail
+    #[test]
+    fn test_invalid_host_and_path() {
+        let tmpdir = TempDir::new().unwrap();
+
+        let mut url = url::Url::parse("sled://foo.example").expect("must parse");
+        url.set_path(tmpdir.path().to_str().unwrap());
+
+        assert!(from_addr(
+            &url.to_string(),
+            gen_blob_service(),
+            gen_directory_service()
+        )
+        .is_err())
+    }
+
     #[tokio::test]
     /// This constructs a GRPC PathInfoService.
     async fn grpc_valid() {
diff --git a/tvix/store/src/pathinfoservice/sled.rs b/tvix/store/src/pathinfoservice/sled.rs
index b389b14b95ad..a12f918c83cf 100644
--- a/tvix/store/src/pathinfoservice/sled.rs
+++ b/tvix/store/src/pathinfoservice/sled.rs
@@ -49,40 +49,6 @@ impl SledPathInfoService {
             directory_service,
         })
     }
-
-    /// Constructs a [SledPathInfoService] from the passed [url::Url]:
-    /// - scheme has to be `sled://`
-    /// - there may not be a host.
-    /// - a path to the sled needs to be provided (which may not be `/`).
-    pub fn from_url(
-        url: &url::Url,
-        blob_service: Arc<dyn BlobService>,
-        directory_service: Arc<dyn DirectoryService>,
-    ) -> Result<Self, Error> {
-        if url.scheme() != "sled" {
-            return Err(Error::StorageError("invalid scheme".to_string()));
-        }
-
-        if url.has_host() {
-            return Err(Error::StorageError(format!(
-                "invalid host: {}",
-                url.host().unwrap()
-            )));
-        }
-
-        // TODO: expose compression and other parameters as URL parameters, drop new and new_temporary?
-        if url.path().is_empty() {
-            Self::new_temporary(blob_service, directory_service)
-                .map_err(|e| Error::StorageError(e.to_string()))
-        } else if url.path() == "/" {
-            Err(Error::StorageError(
-                "cowardly refusing to open / with sled".to_string(),
-            ))
-        } else {
-            Self::new(url.path().into(), blob_service, directory_service)
-                .map_err(|e| Error::StorageError(e.to_string()))
-        }
-    }
 }
 
 #[async_trait]
@@ -172,100 +138,3 @@ impl PathInfoService for SledPathInfoService {
         })))
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use tempfile::TempDir;
-
-    use crate::tests::utils::gen_blob_service;
-    use crate::tests::utils::gen_directory_service;
-
-    use super::SledPathInfoService;
-
-    /// This uses a wrong scheme.
-    #[test]
-    fn test_invalid_scheme() {
-        let url = url::Url::parse("http://foo.example/test").expect("must parse");
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_err()
-        );
-    }
-
-    /// This uses the correct scheme, and doesn't specify a path (temporary sled).
-    #[test]
-    fn test_valid_scheme_temporary() {
-        let url = url::Url::parse("sled://").expect("must parse");
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_ok()
-        );
-    }
-
-    /// This sets the path to a location that doesn't exist, which should fail (as sled doesn't mkdir -p)
-    #[test]
-    fn test_nonexistent_path() {
-        let tmpdir = TempDir::new().unwrap();
-
-        let mut url = url::Url::parse("sled://foo.example").expect("must parse");
-        url.set_path(tmpdir.path().join("foo").join("bar").to_str().unwrap());
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_err()
-        );
-    }
-
-    /// This uses the correct scheme, and specifies / as path (which should fail
-    // for obvious reasons)
-    #[test]
-    fn test_invalid_path_root() {
-        let url = url::Url::parse("sled:///").expect("must parse");
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_err()
-        );
-    }
-
-    /// This uses the correct scheme, and sets a tempdir as location.
-    #[test]
-    fn test_valid_scheme_path() {
-        let tmpdir = TempDir::new().unwrap();
-
-        let mut url = url::Url::parse("sled://").expect("must parse");
-        url.set_path(tmpdir.path().to_str().unwrap());
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_ok()
-        );
-    }
-
-    /// This sets a host, rather than a path, which should fail.
-    #[test]
-    fn test_invalid_host() {
-        let url = url::Url::parse("sled://foo.example").expect("must parse");
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_err()
-        );
-    }
-
-    /// This sets a host AND a valid path, which should fail
-    #[test]
-    fn test_invalid_host_and_path() {
-        let tmpdir = TempDir::new().unwrap();
-
-        let mut url = url::Url::parse("sled://foo.example").expect("must parse");
-        url.set_path(tmpdir.path().to_str().unwrap());
-
-        assert!(
-            SledPathInfoService::from_url(&url, gen_blob_service(), gen_directory_service())
-                .is_err()
-        );
-    }
-}