-
Notifications
You must be signed in to change notification settings - Fork 82
refactor: introduce ObjectStoreExt
trait
#405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -252,11 +252,11 @@ | |||||||
//! | ||||||||
//! # Put Object | ||||||||
//! | ||||||||
//! Use the [`ObjectStore::put`] method to atomically write data. | ||||||||
//! Use the [`ObjectStoreExt::put`] method to atomically write data. | ||||||||
//! | ||||||||
//! ``` | ||||||||
//! # use object_store::local::LocalFileSystem; | ||||||||
//! # use object_store::{ObjectStore, PutPayload}; | ||||||||
//! # use object_store::{ObjectStore, ObjectStoreExt, PutPayload}; | ||||||||
//! # use std::sync::Arc; | ||||||||
//! # use object_store::path::Path; | ||||||||
//! # fn get_object_store() -> Arc<dyn ObjectStore> { | ||||||||
|
@@ -338,7 +338,7 @@ | |||||||
//! | ||||||||
//! ``` | ||||||||
//! # use object_store::local::LocalFileSystem; | ||||||||
//! # use object_store::{ObjectStore, PutPayloadMut}; | ||||||||
//! # use object_store::{ObjectStore, ObjectStoreExt, PutPayloadMut}; | ||||||||
//! # use std::sync::Arc; | ||||||||
//! # use bytes::Bytes; | ||||||||
//! # use tokio::io::AsyncWriteExt; | ||||||||
|
@@ -587,19 +587,22 @@ pub type DynObjectStore = dyn ObjectStore; | |||||||
pub type MultipartId = String; | ||||||||
|
||||||||
/// Universal API to multiple object store services. | ||||||||
/// | ||||||||
/// For more convience methods, check [`ObjectStoreExt`]. | ||||||||
/// | ||||||||
/// # Contract | ||||||||
/// This trait is meant as a contract between object store implementations | ||||||||
/// (e.g. providers, wrappers) and the `object_store` crate itself. | ||||||||
/// | ||||||||
/// The [`ObjectStoreExt`] acts as an API/contract between `object_store` | ||||||||
/// and the store users. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
#[async_trait] | ||||||||
pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static { | ||||||||
/// Save the provided bytes to the specified location | ||||||||
/// Save the provided `payload` to `location` with the given options | ||||||||
/// | ||||||||
/// The operation is guaranteed to be atomic, it will either successfully | ||||||||
/// write the entirety of `payload` to `location`, or fail. No clients | ||||||||
/// should be able to observe a partially written object | ||||||||
async fn put(&self, location: &Path, payload: PutPayload) -> Result<PutResult> { | ||||||||
self.put_opts(location, payload, PutOptions::default()) | ||||||||
.await | ||||||||
} | ||||||||
|
||||||||
/// Save the provided `payload` to `location` with the given options | ||||||||
async fn put_opts( | ||||||||
&self, | ||||||||
location: &Path, | ||||||||
|
@@ -609,7 +612,7 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static { | |||||||
|
||||||||
/// Perform a multipart upload | ||||||||
/// | ||||||||
/// Client should prefer [`ObjectStore::put`] for small payloads, as streaming uploads | ||||||||
/// Client should prefer [`ObjectStoreExt::put`] for small payloads, as streaming uploads | ||||||||
/// typically require multiple separate requests. See [`MultipartUpload`] for more information | ||||||||
/// | ||||||||
/// For more advanced multipart uploads see [`MultipartStore`](multipart::MultipartStore) | ||||||||
|
@@ -620,7 +623,7 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static { | |||||||
|
||||||||
/// Perform a multipart upload with options | ||||||||
/// | ||||||||
/// Client should prefer [`ObjectStore::put`] for small payloads, as streaming uploads | ||||||||
/// Client should prefer [`ObjectStore::put_opts`] for small payloads, as streaming uploads | ||||||||
/// typically require multiple separate requests. See [`MultipartUpload`] for more information | ||||||||
/// | ||||||||
/// For more advanced multipart uploads see [`MultipartStore`](multipart::MultipartStore) | ||||||||
|
@@ -696,7 +699,7 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static { | |||||||
/// # async fn example() -> Result<(), Box<dyn std::error::Error>> { | ||||||||
/// # let root = tempfile::TempDir::new().unwrap(); | ||||||||
/// # let store = LocalFileSystem::new_with_prefix(root.path()).unwrap(); | ||||||||
/// # use object_store::{ObjectStore, ObjectMeta}; | ||||||||
/// # use object_store::{ObjectStore, ObjectStoreExt, ObjectMeta}; | ||||||||
/// # use object_store::path::Path; | ||||||||
/// # use futures::{StreamExt, TryStreamExt}; | ||||||||
/// # | ||||||||
|
@@ -803,10 +806,6 @@ macro_rules! as_ref_impl { | |||||||
($type:ty) => { | ||||||||
#[async_trait] | ||||||||
impl ObjectStore for $type { | ||||||||
async fn put(&self, location: &Path, payload: PutPayload) -> Result<PutResult> { | ||||||||
self.as_ref().put(location, payload).await | ||||||||
} | ||||||||
|
||||||||
async fn put_opts( | ||||||||
&self, | ||||||||
location: &Path, | ||||||||
|
@@ -901,6 +900,40 @@ macro_rules! as_ref_impl { | |||||||
as_ref_impl!(Arc<dyn ObjectStore>); | ||||||||
as_ref_impl!(Box<dyn ObjectStore>); | ||||||||
|
||||||||
/// Helper module to [seal traits](https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/). | ||||||||
mod private { | ||||||||
pub trait Sealed {} | ||||||||
|
||||||||
impl<T> Sealed for T where T: super::ObjectStore + ?Sized {} | ||||||||
} | ||||||||
|
||||||||
/// Extension trait for [`ObjectStore`] with convinience functions. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ease the transition I suggest we augment the documentation here with the following additional information 1. Explain the intended use / goal of this traitAs I understand it, the idea of this trait is to make it more clear what functions must be provided by an Maybe we can also make it clear to implementers that the default implementations of However, I may not understand the full subtely - perhaps we can add more info from #385 as appropriate 2. Add a note about the migration plan / tips to help on upgradeFor example, we could add a section on "upgrade notes" and explain "if you implemented Also it would be good to highlight any planned changes that were forthcoming (like "we plan to move the X, Y and Z functions to ObjectStoreExt over time as well") 3. An exampleI know this sounds silly, but I think especially given There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a motivation text to both traits and also changed the implementation to |
||||||||
/// | ||||||||
/// See "contract" section within the [`ObjectStore`] documentation for more reasoning. | ||||||||
/// | ||||||||
/// # Implementation | ||||||||
/// You MUST NOT implement this trait yourself. It is automatically implemented for all [`ObjectStore`] implementations. | ||||||||
#[async_trait] | ||||||||
pub trait ObjectStoreExt: private::Sealed { | ||||||||
/// Save the provided bytes to the specified location | ||||||||
/// | ||||||||
/// The operation is guaranteed to be atomic, it will either successfully | ||||||||
/// write the entirety of `payload` to `location`, or fail. No clients | ||||||||
/// should be able to observe a partially written object | ||||||||
async fn put(&self, location: &Path, payload: PutPayload) -> Result<PutResult>; | ||||||||
} | ||||||||
|
||||||||
#[async_trait] | ||||||||
impl<T> ObjectStoreExt for T | ||||||||
where | ||||||||
T: ObjectStore + private::Sealed + ?Sized, | ||||||||
{ | ||||||||
async fn put(&self, location: &Path, payload: PutPayload) -> Result<PutResult> { | ||||||||
self.put_opts(location, payload, PutOptions::default()) | ||||||||
.await | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/// Result of a list call that includes objects, prefixes (directories) and a | ||||||||
/// token for the next set of results. Individual result sets may be limited to | ||||||||
/// 1,000 objects based on the underlying object storage's limitations. | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is another key goal that
ObjectStore
is meant to be the minimal required API? If so perhaps we can add something like