maurice/snix
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
Change-Id: I0d905228df086a422bb30322add7236ca41e807b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10026
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>
This commit is contained in:
Florian Klink 2023-11-13 12:35:05 +02:00 • committed by flokli
parent f57fd16d6e
commit 756290a4c0
2 changed files with 81 additions and 136 deletions
Download patch file Download diff file Expand all files Collapse all files

86
tvix/store/src/pathinfoservice/from_addr.rs
View file

@ -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() {

131
tvix/store/src/pathinfoservice/sled.rs
View file

@ -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()
);
}
}