Skip to content

refactor(core): Deprecate OpList::version and add versions instead #5481

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

Merged
merged 7 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bindings/ruby/src/capability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ define_accessors!(Capability, {
list_with_limit: bool,
list_with_start_after: bool,
list_with_recursive: bool,
list_with_version: bool,
list_with_versioned: bool,
presign: bool,
presign_read: bool,
presign_stat: bool,
Expand Down Expand Up @@ -137,7 +137,7 @@ pub fn include(gem_module: &RModule) -> Result<(), Error> {
list_with_limit,
list_with_start_after,
list_with_recursive,
list_with_version,
list_with_versioned,
presign,
presign_read,
presign_stat,
Expand Down
12 changes: 6 additions & 6 deletions core/src/layers/capability_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use std::sync::Arc;
///
/// There are two main differences between this checker with the `CorrectnessChecker`:
/// 1. This checker provides additional checks for capabilities like write_with_content_type and
/// list_with_version, among others. These capabilities do not affect data integrity, even if
/// list_with_versioned, among others. These capabilities do not affect data integrity, even if
/// the underlying storage services do not support them.
///
/// 2. OpenDAL doesn't apply this checker by default. Users can enable this layer if they want to
Expand Down Expand Up @@ -131,7 +131,7 @@ impl<A: Access> LayeredAccess for CapabilityAccessor<A> {

async fn list(&self, path: &str, args: OpList) -> crate::Result<(RpList, Self::Lister)> {
let capability = self.info.full_capability();
if !capability.list_with_version && args.version() {
if !capability.list_with_versioned && args.versioned() {
return Err(new_unsupported_error(
self.info.as_ref(),
Operation::List,
Expand Down Expand Up @@ -191,7 +191,7 @@ impl<A: Access> LayeredAccess for CapabilityAccessor<A> {
args: OpList,
) -> crate::Result<(RpList, Self::BlockingLister)> {
let capability = self.info.full_capability();
if !capability.list_with_version && args.version() {
if !capability.list_with_versioned && args.versioned() {
return Err(new_unsupported_error(
self.info.as_ref(),
Operation::BlockingList,
Expand Down Expand Up @@ -289,16 +289,16 @@ mod tests {
list: true,
..Default::default()
});
let res = op.list_with("path/").version(true).await;
let res = op.list_with("path/").versioned(true).await;
assert!(res.is_err());
assert_eq!(res.unwrap_err().kind(), ErrorKind::Unsupported);

let op = new_test_operator(Capability {
list: true,
list_with_version: true,
list_with_versioned: true,
..Default::default()
});
let res = op.lister_with("path/").version(true).await;
let res = op.lister_with("path/").versioned(true).await;
assert!(res.is_ok())
}
}
21 changes: 17 additions & 4 deletions core/src/raw/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub struct OpList {
/// by the underlying service
///
/// Default to `false`
version: bool,
versioned: bool,
}

impl Default for OpList {
Expand All @@ -121,7 +121,7 @@ impl Default for OpList {
start_after: None,
recursive: false,
concurrent: 1,
version: false,
versioned: false,
}
}
}
Expand Down Expand Up @@ -184,14 +184,27 @@ impl OpList {
}

/// Change the version of this list operation
#[deprecated(since = "0.51.1", note = "use with_versioned instead")]
pub fn with_version(mut self, version: bool) -> Self {
self.version = version;
self.versioned = version;
self
}

/// Change the version of this list operation
pub fn with_versioned(mut self, versioned: bool) -> Self {
self.versioned = versioned;
self
}

/// Get the version of this list operation
#[deprecated(since = "0.51.1", note = "use versioned instead")]
pub fn version(&self) -> bool {
self.version
self.versioned
}

/// Get the version of this list operation
pub fn versioned(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

with_version should be replaced by with_versioned.

self.versioned
}
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/services/s3/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@ impl Access for S3Backend {
list_with_limit: true,
list_with_start_after: true,
list_with_recursive: true,
list_with_version: self.core.enable_versioning,
list_with_versioned: self.core.enable_versioning,
list_has_etag: true,
list_has_content_md5: true,
list_has_content_length: true,
Expand Down Expand Up @@ -1060,7 +1060,7 @@ impl Access for S3Backend {
}

async fn list(&self, path: &str, args: OpList) -> Result<(RpList, Self::Lister)> {
let l = if args.version() {
let l = if args.versioned() {
TwoWays::Two(PageLister::new(S3ObjectVersionsLister::new(
self.core.clone(),
path,
Expand Down
3 changes: 3 additions & 0 deletions core/src/types/capability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,10 @@ pub struct Capability {
/// Indicates if recursive listing is supported.
pub list_with_recursive: bool,
/// Indicates if versioned listing is supported.
#[deprecated(since = "0.51.1", note = "use with_versioned instead")]
pub list_with_version: bool,
/// Indicates if versioned listing is supported.
pub list_with_versioned: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Capability is part of opendal's public API so we can't simplely change it. Please add a new field and deprecate the old one.

/// Indicates whether cache control information is available in list response
pub list_has_cache_control: bool,
/// Indicates whether content disposition information is available in list response
Expand Down
28 changes: 26 additions & 2 deletions core/src/types/operator/operator_futures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,20 @@ impl<F: Future<Output = Result<Vec<Entry>>>> FutureList<F> {
/// by the underlying service
///
/// Default to `false`
#[deprecated(since = "0.51.1", note = "use versioned instead")]
pub fn version(self, v: bool) -> Self {
self.map(|args| args.with_version(v))
self.map(|args| args.with_versioned(v))
}

/// The version is used to control whether the object versions should be returned.
///
/// - If `false`, list operation will not return with object versions
/// - If `true`, list operation will return with object versions if object versioning is supported
/// by the underlying service
///
/// Default to `false`
pub fn versioned(self, v: bool) -> Self {
self.map(|args| args.with_versioned(v))
}
}

Expand Down Expand Up @@ -528,7 +540,19 @@ impl<F: Future<Output = Result<Lister>>> FutureLister<F> {
/// by the underlying service
///
/// Default to `false`
#[deprecated(since = "0.51.1", note = "use versioned instead")]
pub fn version(self, v: bool) -> Self {
self.map(|args| args.with_version(v))
self.map(|args| args.with_versioned(v))
Copy link
Member

Choose a reason for hiding this comment

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

Please add a versioned for it too. Also, this API should be deprecated.

}

/// The version is used to control whether the object versions should be returned.
///
/// - If `false`, list operation will not return with object versions
/// - If `true`, list operation will return with object versions if object versioning is supported
/// by the underlying service
///
/// Default to `false`
pub fn versioned(self, v: bool) -> Self {
self.map(|args| args.with_versioned(v))
}
}
24 changes: 12 additions & 12 deletions core/tests/behavior/async_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ pub fn tests(op: &Operator, tests: &mut Vec<Trial>) {
test_list_file_with_recursive,
test_list_root_with_recursive,
test_remove_all,
test_list_files_with_version,
test_list_with_version_and_limit,
test_list_with_version_and_start_after
test_list_files_with_versioned,
test_list_with_versioned_and_limit,
test_list_with_versioned_and_start_after
))
}

Expand Down Expand Up @@ -575,8 +575,8 @@ pub async fn test_list_only(op: Operator) -> Result<()> {
Ok(())
}

pub async fn test_list_files_with_version(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_version {
pub async fn test_list_files_with_versioned(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_versioned {
return Ok(());
}

Expand All @@ -586,7 +586,7 @@ pub async fn test_list_files_with_version(op: Operator) -> Result<()> {
op.write(file_path.as_str(), "1").await?;
op.write(file_path.as_str(), "2").await?;

let mut ds = op.list_with(parent.as_str()).version(true).await?;
let mut ds = op.list_with(parent.as_str()).versioned(true).await?;
ds.retain(|de| de.path() != parent.as_str());

assert_eq!(ds.len(), 2);
Expand All @@ -608,13 +608,13 @@ pub async fn test_list_files_with_version(op: Operator) -> Result<()> {
}

// listing a directory with version, which contains more object versions than a page can take
pub async fn test_list_with_version_and_limit(op: Operator) -> Result<()> {
pub async fn test_list_with_versioned_and_limit(op: Operator) -> Result<()> {
// Gdrive think that this test is an abuse of their service and redirect us
// to an infinite loop. Let's ignore this test for gdrive.
if op.info().scheme() == Scheme::Gdrive {
return Ok(());
}
if !op.info().full_capability().list_with_version {
if !op.info().full_capability().list_with_versioned {
return Ok(());
}

Expand All @@ -633,7 +633,7 @@ pub async fn test_list_with_version_and_limit(op: Operator) -> Result<()> {
.collect();
expected.push(parent.to_string());

let mut objects = op.lister_with(parent).version(true).limit(5).await?;
let mut objects = op.lister_with(parent).versioned(true).limit(5).await?;
let mut actual = vec![];
while let Some(o) = objects.try_next().await? {
let path = o.path().to_string();
Expand All @@ -648,8 +648,8 @@ pub async fn test_list_with_version_and_limit(op: Operator) -> Result<()> {
Ok(())
}

pub async fn test_list_with_version_and_start_after(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_version {
pub async fn test_list_with_versioned_and_start_after(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_versioned {
return Ok(());
}

Expand All @@ -672,7 +672,7 @@ pub async fn test_list_with_version_and_start_after(op: Operator) -> Result<()>

let mut objects = op
.lister_with(dir)
.version(true)
.versioned(true)
.start_after(&given[2])
.await?;
let mut actual = vec![];
Expand Down
Loading