From 0b131ec6a239b586ac4037cba7b93b2d625711bb Mon Sep 17 00:00:00 2001 From: Keming Date: Sun, 10 Nov 2024 18:14:10 +0800 Subject: [PATCH 1/5] feat(core): add `if_not_exist` in `OpWrite` Signed-off-by: Keming --- core/src/raw/ops.rs | 12 +++++++++++ core/src/services/s3/backend.rs | 1 + core/src/services/s3/core.rs | 6 ++++++ core/src/types/capability.rs | 2 ++ core/src/types/operator/operator.rs | 19 ++++++++++++++++++ core/src/types/operator/operator_futures.rs | 5 +++++ core/tests/behavior/async_write.rs | 22 +++++++++++++++++++++ 7 files changed, 67 insertions(+) diff --git a/core/src/raw/ops.rs b/core/src/raw/ops.rs index d61b0f3b1b6f..a51b0b144e02 100644 --- a/core/src/raw/ops.rs +++ b/core/src/raw/ops.rs @@ -601,6 +601,7 @@ pub struct OpWrite { cache_control: Option, executor: Option, if_none_match: Option, + if_not_exist: Option, user_metadata: Option>, } @@ -697,6 +698,17 @@ impl OpWrite { self.if_none_match.as_deref() } + /// Set the If-Not-Exist of the option + pub fn with_if_not_exist(mut self, b: bool) -> Self { + self.if_not_exist = Some(b); + self + } + + /// Get If-Not-Exist from option + pub fn if_not_exist(&self) -> Option { + self.if_not_exist + } + /// Merge given executor into option. /// /// If executor has already been set, this will do nothing. diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index 5178b930f109..e18125eece12 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -925,6 +925,7 @@ impl Access for S3Backend { write_with_cache_control: true, write_with_content_type: true, write_with_if_none_match: true, + write_with_if_not_exist: true, write_with_user_metadata: true, // The min multipart size of S3 is 5 MiB. diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index 944dac8921b5..99cf3090e40f 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -480,6 +480,12 @@ impl S3Core { req = req.header(IF_NONE_MATCH, if_none_match); } + if let Some(if_not_exist) = args.if_not_exist() { + if if_not_exist { + req = req.header(IF_NONE_MATCH, "*"); + } + } + // Set body let req = req.body(body).map_err(new_request_build_error)?; diff --git a/core/src/types/capability.rs b/core/src/types/capability.rs index 514f7722f19b..88b551b54177 100644 --- a/core/src/types/capability.rs +++ b/core/src/types/capability.rs @@ -101,6 +101,8 @@ pub struct Capability { pub write_with_cache_control: bool, /// If operator supports write with if none match. pub write_with_if_none_match: bool, + /// If operator supports write with if not exist. + pub write_with_if_not_exist: bool, /// If operator supports write with user defined metadata pub write_with_user_metadata: bool, /// write_multi_max_size is the max size that services support in write_multi. diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index e58c60303f2b..6a76ccdf77f1 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1254,6 +1254,25 @@ impl Operator { /// # } /// ``` /// + /// ## `if_not_exist` + /// + /// Set `if_not_exist` for this `write` request. This can be treated as a simplified version + /// of [`OpWrite::if_none_match`]. + /// + /// This feature is used to write a file only when it doesn't exist. If the file already + /// exists, an error with kind [`ErrorKind::ConditionNotMatch`] will be returned. + /// + /// ```no_run + /// # use opendal::{ErrorKind, Result}; + /// use opendal::Operator; + /// # async fn test(op: Operator, etag: &str) -> Result<()> { + /// let bs = b"hello, world!".to_vec(); + /// let res = op.write_with("path/to/file", bs).if_not_exist(true).await; + /// assert!(res.is_err()); + /// assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); + /// # Ok(())} + /// ``` + /// /// # Examples /// /// ``` diff --git a/core/src/types/operator/operator_futures.rs b/core/src/types/operator/operator_futures.rs index fa689417d8f7..f1652bf191fb 100644 --- a/core/src/types/operator/operator_futures.rs +++ b/core/src/types/operator/operator_futures.rs @@ -329,6 +329,11 @@ impl>> FutureWrite { self.map(|(args, options, bs)| (args.with_if_none_match(s), options, bs)) } + /// Set the If-Not-Exist for this operation. + pub fn if_not_exist(self, b: bool) -> Self { + self.map(|(args, options, bs)| (args.with_if_not_exist(b), options, bs)) + } + /// Set the user defined metadata of the op /// /// ## Notes diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 3ccf42e71527..e96cb166eeb7 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -45,6 +45,7 @@ pub fn tests(op: &Operator, tests: &mut Vec) { test_write_with_content_type, test_write_with_content_disposition, test_write_with_if_none_match, + test_write_with_if_not_exist, test_write_with_user_metadata, test_writer_write, test_writer_write_with_overwrite, @@ -646,3 +647,24 @@ pub async fn test_write_with_if_none_match(op: Operator) -> Result<()> { Ok(()) } + +/// Write an file with if_not_exist will get a ConditionNotMatch error if file exists. +pub async fn test_write_with_if_not_exist(op: Operator) -> Result<()> { + if !op.info().full_capability().write_with_if_not_exist { + return Ok(()); + } + + let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); + + op.write(&path, content.clone()) + .await + .expect("write must succeed"); + let res = op + .write_with(&path, content.clone()) + .if_not_exist(true) + .await; + assert!(res.is_err()); + assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); + + Ok(()) +} From 1272ff1ca840ce766b50350c4a22f8cc2e26a55a Mon Sep 17 00:00:00 2001 From: Keming Date: Mon, 11 Nov 2024 21:19:00 +0800 Subject: [PATCH 2/5] change to if_not_exists Signed-off-by: Keming --- core/src/raw/ops.rs | 10 +++++----- core/src/services/s3/core.rs | 6 ++---- core/src/types/operator/operator.rs | 14 ++++++++------ core/src/types/operator/operator_futures.rs | 4 ++-- core/tests/behavior/async_write.rs | 8 ++++---- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/core/src/raw/ops.rs b/core/src/raw/ops.rs index a51b0b144e02..5875160b068f 100644 --- a/core/src/raw/ops.rs +++ b/core/src/raw/ops.rs @@ -601,7 +601,7 @@ pub struct OpWrite { cache_control: Option, executor: Option, if_none_match: Option, - if_not_exist: Option, + if_not_exists: bool, user_metadata: Option>, } @@ -699,14 +699,14 @@ impl OpWrite { } /// Set the If-Not-Exist of the option - pub fn with_if_not_exist(mut self, b: bool) -> Self { - self.if_not_exist = Some(b); + pub fn with_if_not_exists(mut self, b: bool) -> Self { + self.if_not_exists = b; self } /// Get If-Not-Exist from option - pub fn if_not_exist(&self) -> Option { - self.if_not_exist + pub fn if_not_exists(&self) -> bool { + self.if_not_exists } /// Merge given executor into option. diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index 99cf3090e40f..abc97c4453d8 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -480,10 +480,8 @@ impl S3Core { req = req.header(IF_NONE_MATCH, if_none_match); } - if let Some(if_not_exist) = args.if_not_exist() { - if if_not_exist { - req = req.header(IF_NONE_MATCH, "*"); - } + if args.if_not_exists() { + req = req.header(IF_NONE_MATCH, "*"); } // Set body diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index 6a76ccdf77f1..11cc67861cbf 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1254,20 +1254,22 @@ impl Operator { /// # } /// ``` /// - /// ## `if_not_exist` + /// ## `if_not_exists` /// - /// Set `if_not_exist` for this `write` request. This can be treated as a simplified version - /// of [`OpWrite::if_none_match`]. + /// This feature allows to safely write a file only if it does not exist. It is designed + /// to be concurrency-safe, and can be used to a file lock. For storage services that + /// support the `if_not_exist` feature, only one write operation will succeed, while all + /// other attempts will fail. /// - /// This feature is used to write a file only when it doesn't exist. If the file already - /// exists, an error with kind [`ErrorKind::ConditionNotMatch`] will be returned. + /// If the file already exists, an error with kind [`ErrorKind::ConditionNotMatch`] will + /// be returned. /// /// ```no_run /// # use opendal::{ErrorKind, Result}; /// use opendal::Operator; /// # async fn test(op: Operator, etag: &str) -> Result<()> { /// let bs = b"hello, world!".to_vec(); - /// let res = op.write_with("path/to/file", bs).if_not_exist(true).await; + /// let res = op.write_with("path/to/file", bs).if_not_exists(true).await; /// assert!(res.is_err()); /// assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); /// # Ok(())} diff --git a/core/src/types/operator/operator_futures.rs b/core/src/types/operator/operator_futures.rs index f1652bf191fb..17f1cb77c749 100644 --- a/core/src/types/operator/operator_futures.rs +++ b/core/src/types/operator/operator_futures.rs @@ -330,8 +330,8 @@ impl>> FutureWrite { } /// Set the If-Not-Exist for this operation. - pub fn if_not_exist(self, b: bool) -> Self { - self.map(|(args, options, bs)| (args.with_if_not_exist(b), options, bs)) + pub fn if_not_exists(self, b: bool) -> Self { + self.map(|(args, options, bs)| (args.with_if_not_exists(b), options, bs)) } /// Set the user defined metadata of the op diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index e96cb166eeb7..044de888f8d6 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -45,7 +45,7 @@ pub fn tests(op: &Operator, tests: &mut Vec) { test_write_with_content_type, test_write_with_content_disposition, test_write_with_if_none_match, - test_write_with_if_not_exist, + test_write_with_if_not_exists, test_write_with_user_metadata, test_writer_write, test_writer_write_with_overwrite, @@ -648,8 +648,8 @@ pub async fn test_write_with_if_none_match(op: Operator) -> Result<()> { Ok(()) } -/// Write an file with if_not_exist will get a ConditionNotMatch error if file exists. -pub async fn test_write_with_if_not_exist(op: Operator) -> Result<()> { +/// Write an file with if_not_exists will get a ConditionNotMatch error if file exists. +pub async fn test_write_with_if_not_exists(op: Operator) -> Result<()> { if !op.info().full_capability().write_with_if_not_exist { return Ok(()); } @@ -661,7 +661,7 @@ pub async fn test_write_with_if_not_exist(op: Operator) -> Result<()> { .expect("write must succeed"); let res = op .write_with(&path, content.clone()) - .if_not_exist(true) + .if_not_exists(true) .await; assert!(res.is_err()); assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); From ce87b816bd0908a6f74c209fd381127de803d901 Mon Sep 17 00:00:00 2001 From: Keming Date: Tue, 12 Nov 2024 11:43:07 +0800 Subject: [PATCH 3/5] rm if_none_match with * Signed-off-by: Keming --- core/src/types/operator/operator.rs | 2 +- core/tests/behavior/async_write.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index 11cc67861cbf..f84c9a5e9f98 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1247,7 +1247,7 @@ impl Operator { /// use opendal::Operator; /// # async fn test(op: Operator, etag: &str) -> Result<()> { /// let bs = b"hello, world!".to_vec(); - /// let res = op.write_with("path/to/file", bs).if_none_match("*").await; + /// let res = op.write_with("path/to/file", bs).if_none_match(etag).await; /// assert!(res.is_err()); /// assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); /// # Ok(()) diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 044de888f8d6..17fc1edd3d46 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -638,9 +638,12 @@ pub async fn test_write_with_if_none_match(op: Operator) -> Result<()> { op.write(&path, content.clone()) .await .expect("write must succeed"); + + let meta = op.stat(&path).await?; + let res = op .write_with(&path, content.clone()) - .if_none_match("*") + .if_none_match(meta.etag().expect("etag must exist")) .await; assert!(res.is_err()); assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); From 302ea2610a684024e713d53502a3c3b134111515 Mon Sep 17 00:00:00 2001 From: Keming Date: Tue, 12 Nov 2024 11:46:30 +0800 Subject: [PATCH 4/5] fix typo Signed-off-by: Keming --- core/src/services/s3/backend.rs | 2 +- core/src/types/capability.rs | 2 +- core/src/types/operator/operator.rs | 2 +- core/tests/behavior/async_write.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index e18125eece12..ca43d7a36f8f 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -925,7 +925,7 @@ impl Access for S3Backend { write_with_cache_control: true, write_with_content_type: true, write_with_if_none_match: true, - write_with_if_not_exist: true, + write_with_if_not_exists: true, write_with_user_metadata: true, // The min multipart size of S3 is 5 MiB. diff --git a/core/src/types/capability.rs b/core/src/types/capability.rs index 88b551b54177..1e351d69a69f 100644 --- a/core/src/types/capability.rs +++ b/core/src/types/capability.rs @@ -102,7 +102,7 @@ pub struct Capability { /// If operator supports write with if none match. pub write_with_if_none_match: bool, /// If operator supports write with if not exist. - pub write_with_if_not_exist: bool, + pub write_with_if_not_exists: bool, /// If operator supports write with user defined metadata pub write_with_user_metadata: bool, /// write_multi_max_size is the max size that services support in write_multi. diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index f84c9a5e9f98..4668fc8cd6e1 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1258,7 +1258,7 @@ impl Operator { /// /// This feature allows to safely write a file only if it does not exist. It is designed /// to be concurrency-safe, and can be used to a file lock. For storage services that - /// support the `if_not_exist` feature, only one write operation will succeed, while all + /// support the `if_not_exists` feature, only one write operation will succeed, while all /// other attempts will fail. /// /// If the file already exists, an error with kind [`ErrorKind::ConditionNotMatch`] will diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 17fc1edd3d46..109b2e7bc41b 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -653,7 +653,7 @@ pub async fn test_write_with_if_none_match(op: Operator) -> Result<()> { /// Write an file with if_not_exists will get a ConditionNotMatch error if file exists. pub async fn test_write_with_if_not_exists(op: Operator) -> Result<()> { - if !op.info().full_capability().write_with_if_not_exist { + if !op.info().full_capability().write_with_if_not_exists { return Ok(()); } From aecaeeb3f838d7b896ab542b448f355d81b41420 Mon Sep 17 00:00:00 2001 From: Keming Date: Tue, 12 Nov 2024 14:14:15 +0800 Subject: [PATCH 5/5] address commments Signed-off-by: Keming --- core/src/services/s3/backend.rs | 1 - core/src/services/s3/core.rs | 4 ---- core/src/types/operator/operator.rs | 2 -- core/tests/behavior/async_write.rs | 9 ++++++--- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index ca43d7a36f8f..5b523fc72bc0 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -924,7 +924,6 @@ impl Access for S3Backend { write_can_multi: true, write_with_cache_control: true, write_with_content_type: true, - write_with_if_none_match: true, write_with_if_not_exists: true, write_with_user_metadata: true, diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index abc97c4453d8..bc93b46e34b2 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -476,10 +476,6 @@ impl S3Core { req = self.insert_checksum_header(req, &checksum); } - if let Some(if_none_match) = args.if_none_match() { - req = req.header(IF_NONE_MATCH, if_none_match); - } - if args.if_not_exists() { req = req.header(IF_NONE_MATCH, "*"); } diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index 4668fc8cd6e1..84fb6e0aa695 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1237,8 +1237,6 @@ impl Operator { /// /// This feature can be used to check if the file already exists. /// This prevents overwriting of existing objects with identical key names. - /// Users can use *(asterisk) to verify if a file already exists by matching with any ETag. - /// Note: S3 only support use *(asterisk). /// /// If file exists, an error with kind [`ErrorKind::ConditionNotMatch`] will be returned. /// diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 109b2e7bc41b..b51cc2234f38 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -659,9 +659,12 @@ pub async fn test_write_with_if_not_exists(op: Operator) -> Result<()> { let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); - op.write(&path, content.clone()) - .await - .expect("write must succeed"); + let res = op + .write_with(&path, content.clone()) + .if_not_exists(true) + .await; + assert!(res.is_ok()); + let res = op .write_with(&path, content.clone()) .if_not_exists(true)