Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crepererum
Copy link
Contributor

@crepererum crepererum commented Jun 16, 2025

Which issue does this PR close?

Rationale for this change

See #385.

What changes are included in this PR?

  • the trait
  • put method migrated

Are there any user-facing changes?

  1. users may need to import another trait
  2. users no longer can / have to implement put

@crepererum crepererum force-pushed the crepererum/issue_385_a branch 3 times, most recently from 0d3b72f to 8c2710c Compare June 16, 2025 11:03
@crepererum
Copy link
Contributor Author

crepererum commented Jun 16, 2025

Technically the extension crate doesn't need async_trait because it is never dyn-dispatched (it may use a dyn-dispatched ObjectStore though). However our MSRV doesn't support that yet. So there are two options:

  • bump MSRV to 1.75
  • use async_trait for the extension crate as well, resulting in another indirect call. This is likely never going to be noticeable though, since it's not a hot path. Another downside of it is that the docs generated by async_trait aren't so great (cargo doc generates ugly documentation dtolnay/async-trait#213).

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes
users may need to import another trait
users no longer can / have to implement put

I think this makes sense, but will potentially be disruptive to downstream crates (will have to figure out they need now to use ObjectStoreExt).

I think the Rust compiler error messages will do a reasonable job of telling them what is going on, but adding some additional documentation could help make the transition easier.

use async_trait for the extension crate as well, resulting in another indirect call. This is likely never going to be noticeable though, since it's not a hot path

I think this is more consistent with the rest of the crate and would be preferred suggestion

bump MSRV to 1.75

It seems like 1.75 was released 1.5 years ago, which is a long time in Rust terms but maybe we can avoid making the next upgrade too painful

cc @tustvold

src/lib.rs Outdated
&self,
location: &Path,
payload: PutPayload,
) -> impl Future<Output = Result<PutResult>> + Send;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we changed this to return a BoxFuture would it avoid the need to change MSRV?

Perhaps that is what you mean by avoids dyn dispatch.

@@ -901,6 +894,30 @@ macro_rules! as_ref_impl {
as_ref_impl!(Arc<dyn ObjectStore>);
as_ref_impl!(Box<dyn ObjectStore>);

/// Extension trait for [`ObjectStore`] with convinience functions.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 trait

As I understand it, the idea of this trait is to make it more clear what functions must be provided by an ObjectStore implementation and which are mostly implemented in terms of the others.

Maybe we can also make it clear to implementers that the default implementations of ObjectStoreExt may not be optimal for their uscase

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 upgrade

For example, we could add a section on "upgrade notes" and explain "if you implemented put for your ObjectStore, if it has an implementation different than the default move that implementation to ObjectStoreExt.

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 example

I know this sounds silly, but I think especially given impl Future --> ... signature it is non obvious to the causal Rust programmer that this means the impl should have an async fn

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 async_trait. Does this look more useful to you?

@crepererum crepererum force-pushed the crepererum/issue_385_a branch from 8c2710c to 2341010 Compare June 17, 2025 13:06
@crepererum crepererum force-pushed the crepererum/issue_385_a branch from 2341010 to 0f861bc Compare June 17, 2025 13:12
@crepererum crepererum requested a review from tustvold June 17, 2025 15:11
@crepererum
Copy link
Contributor Author

I plan to migrate other methods in follow-up PRs. I just wanted to keep this PR rather simple.

Comment on lines +594 to +595
/// This trait is meant as a contract between object store implementations
/// (e.g. providers, wrappers) and the `object_store` crate itself.
Copy link
Contributor

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

Suggested change
/// This trait is meant as a contract between object store implementations
/// (e.g. providers, wrappers) and the `object_store` crate itself.
/// This trait is meant as a contract between object store implementations
/// (e.g. providers, wrappers) and the `object_store` crate itself and is
/// intended to be the minimum API required for an object store.

/// (e.g. providers, wrappers) and the `object_store` crate itself.
///
/// The [`ObjectStoreExt`] acts as an API/contract between `object_store`
/// and the store users.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// and the store users.
/// and the store users and provides additional methods that may be simpler to use but overlap
/// in functionality with `ObjectStore`

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken a quick look, unfortunately very short on time this week.

I think the major question from me is if this is premature, in particular we don't currently have a list_opts or copy_opts method, and in the former case it isn't immediately obvious what this would look like. Similarly methods like get_ranges have specializations that make a material difference.

The reason I've held off doing something like this before is that some methods are not obvious how they'd fit into this, I'd feel more comfortable to have a plan for these before committing to this approach

@crepererum
Copy link
Contributor Author

Taken a quick look, unfortunately very short on time this week.

I think the major question from me is if this is premature, in particular we don't currently have a list_opts or copy_opts method, and in the former case it isn't immediately obvious what this would look like. Similarly methods like get_ranges have specializations that make a material difference.

The reason I've held off doing something like this before is that some methods are not obvious how they'd fit into this, I'd feel more comfortable to have a plan for these before committing to this approach

I think we'll likely cover the following methods via _opts:

  • get (except "rangeS")
  • put
  • put multipart
  • copy
  • rename

IMHO that are enough methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants