Skip to content
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

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

Merged
merged 7 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from all 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_versions: 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_versions,
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_versions, 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_versions && args.versions() {
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_versions && args.versions() {
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/").versions(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_versions: true,
..Default::default()
});
let res = op.lister_with("path/").version(true).await;
let res = op.lister_with("path/").versions(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,
versions: bool,
}

impl Default for OpList {
Expand All @@ -121,7 +121,7 @@ impl Default for OpList {
start_after: None,
recursive: false,
concurrent: 1,
version: false,
versions: 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_versions instead")]
pub fn with_version(mut self, version: bool) -> Self {
self.version = version;
self.versions = version;
self
}

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

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

/// Get the version of this list operation
pub fn versions(&self) -> bool {
self.versions
}
}

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_versions: 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.versions() {
TwoWays::Two(PageLister::new(S3ObjectVersionsLister::new(
self.core.clone(),
path,
Expand Down
11 changes: 7 additions & 4 deletions core/src/types/capability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub struct Capability {
pub stat_with_override_content_disposition: bool,
/// Indicates if Content-Type header override is supported during stat operations.
pub stat_with_override_content_type: bool,
/// Indicates if versioned stat operations are supported.
/// Indicates if versions stat operations are supported.
pub stat_with_version: bool,
/// Indicates whether cache control information is available in stat response
pub stat_has_cache_control: bool,
Expand Down Expand Up @@ -113,7 +113,7 @@ pub struct Capability {
pub read_with_override_content_disposition: bool,
/// Indicates if Content-Type header override is supported during read operations.
pub read_with_override_content_type: bool,
/// Indicates if versioned read operations are supported.
/// Indicates if versions read operations are supported.
pub read_with_version: bool,

/// Indicates if the operator supports write operations.
Expand Down Expand Up @@ -155,7 +155,7 @@ pub struct Capability {

/// Indicates if delete operations are supported.
pub delete: bool,
/// Indicates if versioned delete operations are supported.
/// Indicates if versions delete operations are supported.
pub delete_with_version: bool,
/// Maximum size supported for single delete operations.
pub delete_max_size: Option<usize>,
Expand All @@ -174,8 +174,11 @@ pub struct Capability {
pub list_with_start_after: bool,
/// Indicates if recursive listing is supported.
pub list_with_recursive: bool,
/// Indicates if versioned listing is supported.
/// Indicates if versions listing is supported.
#[deprecated(since = "0.51.1", note = "use with_versions instead")]
pub list_with_version: bool,
/// Indicates if versions listing is supported.
pub list_with_versions: bool,
/// 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 versions instead")]
pub fn version(self, v: bool) -> Self {
self.map(|args| args.with_version(v))
self.map(|args| args.with_versions(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 versions(self, v: bool) -> Self {
self.map(|args| args.with_versions(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 versions instead")]
pub fn version(self, v: bool) -> Self {
self.map(|args| args.with_version(v))
self.map(|args| args.with_versions(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 versions(self, v: bool) -> Self {
self.map(|args| args.with_versions(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_versions,
test_list_with_versions_and_limit,
test_list_with_versions_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_versions(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_versions {
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()).versions(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_versions_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_versions {
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).versions(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_versions_and_start_after(op: Operator) -> Result<()> {
if !op.info().full_capability().list_with_versions {
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)
.versions(true)
.start_after(&given[2])
.await?;
let mut actual = vec![];
Expand Down
Loading