From 7209d9926b3e0c09c6f417eb56a9fd8c312d5f83 Mon Sep 17 00:00:00 2001 From: cosmod Date: Thu, 17 Oct 2024 11:29:56 +0800 Subject: [PATCH 1/3] refactor(types/operator): rename is_exist to exists --- .../src/main.rs | 2 +- core/src/docs/rfcs/3243_list_prefix.md | 2 +- core/src/types/operator/blocking_operator.rs | 25 +++++++++++++++++ core/src/types/operator/operator.rs | 27 +++++++++++++++++++ core/tests/behavior/async_delete.rs | 12 ++++----- core/tests/behavior/async_list.rs | 2 +- core/tests/behavior/async_write.rs | 4 +-- core/tests/behavior/blocking_delete.rs | 4 +-- core/tests/behavior/blocking_list.rs | 2 +- 9 files changed, 66 insertions(+), 14 deletions(-) diff --git a/core/edge/s3_aws_assume_role_with_web_identity/src/main.rs b/core/edge/s3_aws_assume_role_with_web_identity/src/main.rs index b5a5bc64883..367e786ce21 100644 --- a/core/edge/s3_aws_assume_role_with_web_identity/src/main.rs +++ b/core/edge/s3_aws_assume_role_with_web_identity/src/main.rs @@ -25,7 +25,7 @@ async fn main() -> Result<()> { assert_eq!(op.info().scheme(), Scheme::S3); let result = op - .is_exist(&uuid::Uuid::new_v4().to_string()) + .exists(&uuid::Uuid::new_v4().to_string()) .await .expect("this operation should never return error"); assert!(!result, "the file must be not exist"); diff --git a/core/src/docs/rfcs/3243_list_prefix.md b/core/src/docs/rfcs/3243_list_prefix.md index 653ae518a51..1d742fe33a7 100644 --- a/core/src/docs/rfcs/3243_list_prefix.md +++ b/core/src/docs/rfcs/3243_list_prefix.md @@ -16,7 +16,7 @@ As a side-effect of this design, OpenDAL always return exist for `stat("not_exis There are some issues and pull requests related to those issues. - [Invalid metadata for dir objects in s3](https://github.com/apache/opendal/issues/3199) -- [`is_exist` always return true for key end with '/', in S3 service](https://github.com/apache/opendal/issues/2086) +- [`exists` always return true for key end with '/', in S3 service](https://github.com/apache/opendal/issues/2086) POSIX-like file systems also have their own issues, as they lack native support for listing a prefix. diff --git a/core/src/types/operator/blocking_operator.rs b/core/src/types/operator/blocking_operator.rs index 3e5c5bab8f8..47f48d01651 100644 --- a/core/src/types/operator/blocking_operator.rs +++ b/core/src/types/operator/blocking_operator.rs @@ -267,6 +267,30 @@ impl BlockingOperator { )) } + /// Check if this path exists or not. + /// + /// # Example + /// + /// ```no_run + /// use anyhow::Result; + /// use opendal::BlockingOperator; + /// fn test(op: BlockingOperator) -> Result<()> { + /// let _ = op.exists("test")?; + /// + /// Ok(()) + /// } + /// ``` + pub fn exists(&self, path: &str) -> Result { + let r = self.stat(path); + match r { + Ok(_) => Ok(true), + Err(err) => match err.kind() { + ErrorKind::NotFound => Ok(false), + _ => Err(err), + }, + } + } + /// Check if this path exists or not. /// /// # Example @@ -280,6 +304,7 @@ impl BlockingOperator { /// Ok(()) /// } /// ``` + #[deprecated(note = "rename to `exists` for consistence with `std::fs::exists`")] pub fn is_exist(&self, path: &str) -> Result { let r = self.stat(path); match r { diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index e2451a3ad27..e58c60303f2 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -330,6 +330,32 @@ impl Operator { ) } + /// Check if this path exists or not. + /// + /// # Example + /// + /// ``` + /// use anyhow::Result; + /// use futures::io; + /// use opendal::Operator; + /// + /// async fn test(op: Operator) -> Result<()> { + /// let _ = op.exists("test").await?; + /// + /// Ok(()) + /// } + /// ``` + pub async fn exists(&self, path: &str) -> Result { + let r = self.stat(path).await; + match r { + Ok(_) => Ok(true), + Err(err) => match err.kind() { + ErrorKind::NotFound => Ok(false), + _ => Err(err), + }, + } + } + /// Check if this path exists or not. /// /// # Example @@ -345,6 +371,7 @@ impl Operator { /// Ok(()) /// } /// ``` + #[deprecated(note = "rename to `exists` for consistence with `std::fs::exists`")] pub async fn is_exist(&self, path: &str) -> Result { let r = self.stat(path).await; match r { diff --git a/core/tests/behavior/async_delete.rs b/core/tests/behavior/async_delete.rs index 0225f50ab66..9f055b67a48 100644 --- a/core/tests/behavior/async_delete.rs +++ b/core/tests/behavior/async_delete.rs @@ -55,7 +55,7 @@ pub async fn test_delete_file(op: Operator) -> Result<()> { op.delete(&path).await?; // Stat it again to check. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); Ok(()) } @@ -96,7 +96,7 @@ pub async fn test_delete_with_special_chars(op: Operator) -> Result<()> { op.delete(&path).await?; // Stat it again to check. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); Ok(()) } @@ -121,7 +121,7 @@ pub async fn test_remove_one_file(op: Operator) -> Result<()> { op.remove(vec![path.clone()]).await?; // Stat it again to check. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); op.write(&format!("/{path}"), content) .await @@ -130,7 +130,7 @@ pub async fn test_remove_one_file(op: Operator) -> Result<()> { op.remove(vec![path.clone()]).await?; // Stat it again to check. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); Ok(()) } @@ -163,7 +163,7 @@ pub async fn test_delete_stream(op: Operator) -> Result<()> { // Stat it again to check. for path in expected.iter() { assert!( - !op.is_exist(&format!("{dir}/{path}")).await?, + !op.exists(&format!("{dir}/{path}")).await?, "{path} should be removed" ) } @@ -229,7 +229,7 @@ pub async fn test_delete_with_version(op: Operator) -> Result<()> { let version = meta.version().expect("must have version"); op.delete(path.as_str()).await.expect("delete must success"); - assert!(!op.is_exist(path.as_str()).await?); + assert!(!op.exists(path.as_str()).await?); // After a simple delete, the data can still be accessed using its version. let meta = op diff --git a/core/tests/behavior/async_list.rs b/core/tests/behavior/async_list.rs index ba862f70252..69157cdc155 100644 --- a/core/tests/behavior/async_list.rs +++ b/core/tests/behavior/async_list.rs @@ -655,7 +655,7 @@ pub async fn test_remove_all(op: Operator) -> Result<()> { continue; } assert!( - !op.is_exist(&format!("{parent}/{path}")).await?, + !op.exists(&format!("{parent}/{path}")).await?, "{parent}/{path} should be removed" ) } diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 9bfdc15059a..3ccf42e7152 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -255,7 +255,7 @@ pub async fn test_writer_abort(op: Operator) -> Result<()> { } // Aborted writer should not write actual file. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); Ok(()) } @@ -282,7 +282,7 @@ pub async fn test_writer_abort_with_concurrent(op: Operator) -> Result<()> { } // Aborted writer should not write actual file. - assert!(!op.is_exist(&path).await?); + assert!(!op.exists(&path).await?); Ok(()) } diff --git a/core/tests/behavior/blocking_delete.rs b/core/tests/behavior/blocking_delete.rs index f5d1dd31699..79e0ef24db1 100644 --- a/core/tests/behavior/blocking_delete.rs +++ b/core/tests/behavior/blocking_delete.rs @@ -52,7 +52,7 @@ pub fn test_blocking_delete_file(op: BlockingOperator) -> Result<()> { op.delete(&path)?; // Stat it again to check. - assert!(!op.is_exist(&path)?); + assert!(!op.exists(&path)?); Ok(()) } @@ -67,7 +67,7 @@ pub fn test_blocking_remove_one_file(op: BlockingOperator) -> Result<()> { op.remove(vec![path.clone()])?; // Stat it again to check. - assert!(!op.is_exist(&path)?); + assert!(!op.exists(&path)?); Ok(()) } diff --git a/core/tests/behavior/blocking_list.rs b/core/tests/behavior/blocking_list.rs index dd0647090ef..225d19cad49 100644 --- a/core/tests/behavior/blocking_list.rs +++ b/core/tests/behavior/blocking_list.rs @@ -196,7 +196,7 @@ pub fn test_blocking_remove_all(op: BlockingOperator) -> Result<()> { continue; } assert!( - !op.is_exist(&format!("{parent}/{path}"))?, + !op.exists(&format!("{parent}/{path}"))?, "{parent}/{path} should be removed" ) } From 9e9e9b65878ffab257f926f375e9a64123a631a1 Mon Sep 17 00:00:00 2001 From: cosmod Date: Thu, 17 Oct 2024 11:52:43 +0800 Subject: [PATCH 2/3] refactor(integrations): use exists instead of is_exist --- integrations/dav-server/src/fs.rs | 4 ++-- integrations/parquet/src/async_writer.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/integrations/dav-server/src/fs.rs b/integrations/dav-server/src/fs.rs index a1096bf47ad..80444d8c676 100644 --- a/integrations/dav-server/src/fs.rs +++ b/integrations/dav-server/src/fs.rs @@ -119,7 +119,7 @@ impl DavFileSystem for OpendalFs { // During MKCOL processing, a server MUST make the Request-URI a member of its parent collection, unless the Request-URI is "/". If no such ancestor exists, the method MUST fail. // refer to https://datatracker.ietf.org/doc/html/rfc2518#section-8.3.1 let parent = Path::new(&path).parent().unwrap(); - match self.op.is_exist(parent.to_str().unwrap()).await { + match self.op.exists(parent.to_str().unwrap()).await { Ok(exist) => { if !exist && parent != Path::new("/") { return Err(FsError::NotFound); @@ -132,7 +132,7 @@ impl DavFileSystem for OpendalFs { let path = path.as_str(); // check if the given path is exist (MKCOL on existing collection should fail (RFC2518:8.3.1)) - let exist = self.op.is_exist(path).await; + let exist = self.op.exists(path).await; match exist { Ok(exist) => match exist { true => Err(FsError::Exists), diff --git a/integrations/parquet/src/async_writer.rs b/integrations/parquet/src/async_writer.rs index 47266564c39..7f4ab16e5c3 100644 --- a/integrations/parquet/src/async_writer.rs +++ b/integrations/parquet/src/async_writer.rs @@ -137,7 +137,7 @@ mod tests { writer.write(bytes).await.unwrap(); drop(writer); - let exist = op.is_exist(path).await.unwrap(); + let exist = op.exists(path).await.unwrap(); assert!(!exist); } From 23696cec0d90f1cf18de492cdea1cae30c320b28 Mon Sep 17 00:00:00 2001 From: cosmod Date: Thu, 17 Oct 2024 14:59:04 +0800 Subject: [PATCH 3/3] fix: keep is_exist unchanged in rfcs --- core/src/docs/rfcs/3243_list_prefix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/docs/rfcs/3243_list_prefix.md b/core/src/docs/rfcs/3243_list_prefix.md index 1d742fe33a7..653ae518a51 100644 --- a/core/src/docs/rfcs/3243_list_prefix.md +++ b/core/src/docs/rfcs/3243_list_prefix.md @@ -16,7 +16,7 @@ As a side-effect of this design, OpenDAL always return exist for `stat("not_exis There are some issues and pull requests related to those issues. - [Invalid metadata for dir objects in s3](https://github.com/apache/opendal/issues/3199) -- [`exists` always return true for key end with '/', in S3 service](https://github.com/apache/opendal/issues/2086) +- [`is_exist` always return true for key end with '/', in S3 service](https://github.com/apache/opendal/issues/2086) POSIX-like file systems also have their own issues, as they lack native support for listing a prefix.