From 80039ea5425ec6b80258c37f96550d5b8c820d2e Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Wed, 27 Nov 2024 09:24:32 -0800 Subject: [PATCH] Move Repository::address to dedicated trait This makes it possible for other things to have a trait bound on `Address` instead of all of `Repository`, which means other types can implement `Address` without having to also implement all of `Repository` (not shown in this commit). Also change the method signature to return `Cow` since there are cases where an existing `Url` value can be borrowed. Signed-off-by: J Robert Ray --- crates/spfs/src/bootstrap.rs | 2 +- crates/spfs/src/runtime/storage.rs | 4 ++-- crates/spfs/src/storage/address.rs | 17 +++++++++++++++++ crates/spfs/src/storage/fallback/repository.rs | 13 ++++++++----- crates/spfs/src/storage/fs/repository.rs | 14 ++++++++------ crates/spfs/src/storage/handle.rs | 16 ++++++++++------ crates/spfs/src/storage/mod.rs | 2 ++ crates/spfs/src/storage/pinned/repository.rs | 10 ++++++---- crates/spfs/src/storage/prelude.rs | 1 + crates/spfs/src/storage/proxy/repository.rs | 7 ++++--- crates/spfs/src/storage/repository.rs | 10 ++-------- crates/spfs/src/storage/rpc/repository.rs | 10 +++++++--- crates/spfs/src/storage/tar/repository.rs | 9 ++++++--- crates/spk-storage/src/storage/spfs.rs | 8 ++++---- 14 files changed, 78 insertions(+), 45 deletions(-) create mode 100644 crates/spfs/src/storage/address.rs diff --git a/crates/spfs/src/bootstrap.rs b/crates/spfs/src/bootstrap.rs index 5836fcc41c..eb373d4135 100644 --- a/crates/spfs/src/bootstrap.rs +++ b/crates/spfs/src/bootstrap.rs @@ -254,7 +254,7 @@ where pub(crate) fn build_spfs_remove_durable_command( runtime_name: String, - repo_address: Url, + repo_address: &Url, ) -> Result { let exe = match which_spfs("clean") { None => return Err(Error::MissingBinary("spfs-clean")), diff --git a/crates/spfs/src/runtime/storage.rs b/crates/spfs/src/runtime/storage.rs index 5b94025b8a..e0f939aeb4 100644 --- a/crates/spfs/src/runtime/storage.rs +++ b/crates/spfs/src/runtime/storage.rs @@ -918,7 +918,7 @@ impl Storage { } /// The address of the underlying repository being used - pub fn address(&self) -> url::Url { + pub fn address(&self) -> Cow<'_, url::Url> { self.inner.address() } @@ -935,7 +935,7 @@ impl Storage { // is run to do it. let mut cmd = bootstrap::build_spfs_remove_durable_command( name.as_ref().to_string(), - self.inner.address(), + &self.inner.address(), )? .into_std(); tracing::trace!("running: {cmd:?}"); diff --git a/crates/spfs/src/storage/address.rs b/crates/spfs/src/storage/address.rs new file mode 100644 index 0000000000..d55f66fe1b --- /dev/null +++ b/crates/spfs/src/storage/address.rs @@ -0,0 +1,17 @@ +// Copyright (c) Contributors to the SPK project. +// SPDX-License-Identifier: Apache-2.0 +// https://github.com/spkenv/spk + +use std::borrow::Cow; + +/// The address of a repository. +pub trait Address { + /// Return the address of this repository. + fn address(&self) -> Cow<'_, url::Url>; +} + +impl Address for &T { + fn address(&self) -> Cow<'_, url::Url> { + T::address(&**self) + } +} diff --git a/crates/spfs/src/storage/fallback/repository.rs b/crates/spfs/src/storage/fallback/repository.rs index 1521292499..6de26c4a65 100644 --- a/crates/spfs/src/storage/fallback/repository.rs +++ b/crates/spfs/src/storage/fallback/repository.rs @@ -366,8 +366,8 @@ impl BlobStorage for FallbackProxy {} impl ManifestStorage for FallbackProxy {} impl LayerStorage for FallbackProxy {} impl PlatformStorage for FallbackProxy {} -impl Repository for FallbackProxy { - fn address(&self) -> url::Url { +impl Address for FallbackProxy { + fn address(&self) -> Cow<'_, url::Url> { let config = Config { primary: self.primary.address().to_string(), secondary: self @@ -376,11 +376,14 @@ impl Repository for FallbackProxy { .map(|s| s.address().to_string()) .collect(), }; - config - .to_address() - .expect("We should not fail to create a url") + Cow::Owned( + config + .to_address() + .expect("We should not fail to create a url"), + ) } } +impl Repository for FallbackProxy {} impl LocalRepository for FallbackProxy { #[inline] diff --git a/crates/spfs/src/storage/fs/repository.rs b/crates/spfs/src/storage/fs/repository.rs index 83993c7c75..a90daf4a19 100644 --- a/crates/spfs/src/storage/fs/repository.rs +++ b/crates/spfs/src/storage/fs/repository.rs @@ -262,11 +262,12 @@ impl BlobStorage for FsRepository {} impl ManifestStorage for FsRepository {} impl LayerStorage for FsRepository {} impl PlatformStorage for FsRepository {} -impl Repository for FsRepository { - fn address(&self) -> url::Url { - url::Url::from_directory_path(self.root()).unwrap() +impl Address for FsRepository { + fn address(&self) -> Cow<'_, url::Url> { + Cow::Owned(url::Url::from_directory_path(self.root()).unwrap()) } } +impl Repository for FsRepository {} impl std::fmt::Debug for FsRepository { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -538,11 +539,12 @@ impl BlobStorage for OpenFsRepository {} impl ManifestStorage for OpenFsRepository {} impl LayerStorage for OpenFsRepository {} impl PlatformStorage for OpenFsRepository {} -impl Repository for OpenFsRepository { - fn address(&self) -> url::Url { - url::Url::from_directory_path(self.root()).unwrap() +impl Address for OpenFsRepository { + fn address(&self) -> Cow<'_, url::Url> { + Cow::Owned(url::Url::from_directory_path(self.root()).unwrap()) } } +impl Repository for OpenFsRepository {} impl std::fmt::Debug for OpenFsRepository { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { diff --git a/crates/spfs/src/storage/handle.rs b/crates/spfs/src/storage/handle.rs index 098a1f602f..12c44d5a1d 100644 --- a/crates/spfs/src/storage/handle.rs +++ b/crates/spfs/src/storage/handle.rs @@ -36,12 +36,14 @@ macro_rules! each_variant { }; } -#[async_trait::async_trait] -impl Repository for RepositoryHandle { - fn address(&self) -> url::Url { +impl Address for RepositoryHandle { + fn address(&self) -> Cow<'_, url::Url> { each_variant!(self, repo, { repo.address() }) } +} +#[async_trait::async_trait] +impl Repository for RepositoryHandle { async fn has_ref(&self, reference: &str) -> bool { each_variant!(self, repo, { repo.has_ref(reference).await }) } @@ -247,13 +249,15 @@ impl Database for RepositoryHandle { } } -#[async_trait::async_trait] -impl Repository for Arc { +impl Address for Arc { /// Return the address of this repository. - fn address(&self) -> url::Url { + fn address(&self) -> Cow<'_, url::Url> { each_variant!(&**self, repo, { repo.address() }) } +} +#[async_trait::async_trait] +impl Repository for Arc { /// Return true if this repository contains the given reference. async fn has_ref(&self, reference: &str) -> bool { each_variant!(&**self, repo, { repo.has_ref(reference).await }) diff --git a/crates/spfs/src/storage/mod.rs b/crates/spfs/src/storage/mod.rs index ec856ce0d6..673ad6d356 100644 --- a/crates/spfs/src/storage/mod.rs +++ b/crates/spfs/src/storage/mod.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk +mod address; mod blob; mod error; mod layer; @@ -24,6 +25,7 @@ pub mod tar; use std::sync::Arc; +pub use address::Address; pub use blob::BlobStorage; use chrono::{DateTime, Utc}; pub use error::OpenRepositoryError; diff --git a/crates/spfs/src/storage/pinned/repository.rs b/crates/spfs/src/storage/pinned/repository.rs index 9690f95c95..bf7a33ee42 100644 --- a/crates/spfs/src/storage/pinned/repository.rs +++ b/crates/spfs/src/storage/pinned/repository.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk +use std::borrow::Cow; use std::pin::Pin; use std::sync::Arc; @@ -153,17 +154,18 @@ impl BlobStorage for PinnedRepository where T: BlobStorage + 'static {} impl ManifestStorage for PinnedRepository where T: ManifestStorage + 'static {} impl LayerStorage for PinnedRepository where T: LayerStorage + 'static {} impl PlatformStorage for PinnedRepository where T: PlatformStorage + 'static {} -impl Repository for PinnedRepository +impl Address for PinnedRepository where T: Repository + 'static, { - fn address(&self) -> url::Url { - let mut base = self.inner.address(); + fn address(&self) -> Cow<'_, url::Url> { + let mut base = self.inner.address().into_owned(); base.query_pairs_mut() .append_pair("when", &self.pin.to_string()); - base + Cow::Owned(base) } } +impl Repository for PinnedRepository where T: Repository + 'static {} impl std::fmt::Debug for PinnedRepository where diff --git a/crates/spfs/src/storage/prelude.rs b/crates/spfs/src/storage/prelude.rs index 2b8a421a6d..278f9a259e 100644 --- a/crates/spfs/src/storage/prelude.rs +++ b/crates/spfs/src/storage/prelude.rs @@ -4,6 +4,7 @@ pub use super::config::{FromConfig, FromUrl}; pub use super::{ + Address, BlobStorage, LayerStorage, ManifestStorage, diff --git a/crates/spfs/src/storage/proxy/repository.rs b/crates/spfs/src/storage/proxy/repository.rs index 17d842af87..c59f4e132d 100644 --- a/crates/spfs/src/storage/proxy/repository.rs +++ b/crates/spfs/src/storage/proxy/repository.rs @@ -335,8 +335,8 @@ impl BlobStorage for ProxyRepository {} impl ManifestStorage for ProxyRepository {} impl LayerStorage for ProxyRepository {} impl PlatformStorage for ProxyRepository {} -impl Repository for ProxyRepository { - fn address(&self) -> url::Url { +impl Address for ProxyRepository { + fn address(&self) -> Cow<'_, url::Url> { let config = Config { primary: self.primary.address().to_string(), secondary: self @@ -345,6 +345,7 @@ impl Repository for ProxyRepository { .map(|s| s.address().to_string()) .collect(), }; - config.to_address().expect("config creates a valid url") + Cow::Owned(config.to_address().expect("config creates a valid url")) } } +impl Repository for ProxyRepository {} diff --git a/crates/spfs/src/storage/repository.rs b/crates/spfs/src/storage/repository.rs index 147327b5f1..984a63d7d4 100644 --- a/crates/spfs/src/storage/repository.rs +++ b/crates/spfs/src/storage/repository.rs @@ -39,7 +39,8 @@ impl std::fmt::Display for Ref { /// Represents a storage location for spfs data. #[async_trait] pub trait Repository: - super::TagStorage + super::Address + + super::TagStorage + super::PayloadStorage + super::ManifestStorage + super::BlobStorage @@ -51,9 +52,6 @@ pub trait Repository: + Send + Sync { - /// Return the address of this repository. - fn address(&self) -> url::Url; - /// Return true if this repository contains the given reference. async fn has_ref(&self, reference: &str) -> bool { self.read_ref(reference).await.is_ok() @@ -115,10 +113,6 @@ pub trait Repository: #[async_trait::async_trait] impl Repository for &T { - fn address(&self) -> url::Url { - Repository::address(&**self) - } - async fn has_ref(&self, reference: &str) -> bool { (**self).has_ref(reference).await } diff --git a/crates/spfs/src/storage/rpc/repository.rs b/crates/spfs/src/storage/rpc/repository.rs index d3337ad1df..1cc66afb03 100644 --- a/crates/spfs/src/storage/rpc/repository.rs +++ b/crates/spfs/src/storage/rpc/repository.rs @@ -2,6 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk +use std::borrow::Cow; + use storage::FromUrl; use crate::config::ToAddress; @@ -168,8 +170,10 @@ impl RpcRepository { } } -impl storage::Repository for RpcRepository { - fn address(&self) -> url::Url { - self.address.clone() +impl storage::Address for RpcRepository { + fn address(&self) -> Cow<'_, url::Url> { + Cow::Borrowed(&self.address) } } + +impl storage::Repository for RpcRepository {} diff --git a/crates/spfs/src/storage/tar/repository.rs b/crates/spfs/src/storage/tar/repository.rs index bae29cf99c..32487fe28d 100644 --- a/crates/spfs/src/storage/tar/repository.rs +++ b/crates/spfs/src/storage/tar/repository.rs @@ -410,8 +410,11 @@ impl BlobStorage for TarRepository {} impl ManifestStorage for TarRepository {} impl LayerStorage for TarRepository {} impl PlatformStorage for TarRepository {} -impl Repository for TarRepository { - fn address(&self) -> url::Url { - url::Url::from_file_path(&self.repo_dir).expect("unexpected failure creating url") +impl Address for TarRepository { + fn address(&self) -> Cow<'_, url::Url> { + Cow::Owned( + url::Url::from_file_path(&self.repo_dir).expect("unexpected failure creating url"), + ) } } +impl Repository for TarRepository {} diff --git a/crates/spk-storage/src/storage/spfs.rs b/crates/spk-storage/src/storage/spfs.rs index 984cce7532..fccdd5142a 100644 --- a/crates/spk-storage/src/storage/spfs.rs +++ b/crates/spk-storage/src/storage/spfs.rs @@ -159,7 +159,7 @@ where name_and_repo: NameAndRepositoryWithTagStrategy, ) -> Result { let inner = name_and_repo.repo.into(); - let address = inner.address(); + let address = inner.address().into_owned(); Ok(Self { caches: CachesForAddress::new(&address), address, @@ -175,7 +175,7 @@ where impl SpfsRepository { pub async fn new(name: &str, address: &str) -> Result { let inner = spfs::open_repository(address).await?; - let address = inner.address(); + let address = inner.address().into_owned(); Ok(Self { caches: CachesForAddress::new(&address), address, @@ -1313,7 +1313,7 @@ pub async fn local_repository() -> Result> let config = spfs::get_config()?; let repo = config.get_local_repository().await?; let inner: spfs::prelude::RepositoryHandle = repo.into(); - let address = inner.address(); + let address = inner.address().into_owned(); Ok(SpfsRepository { caches: CachesForAddress::new(&address), address, @@ -1333,7 +1333,7 @@ pub async fn remote_repository, TagStrategy>( ) -> Result> { let config = spfs::get_config()?; let inner = config.get_remote(&name).await?; - let address = inner.address(); + let address = inner.address().into_owned(); Ok(SpfsRepository { caches: CachesForAddress::new(&address), address,