From fba5fc9b42e878d85df78303e6bed410594437d9 Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Wed, 27 Nov 2024 02:44:14 +0000 Subject: [PATCH 1/7] feat(services/s3): add if-match to `OpWrite` --- core/src/raw/ops.rs | 12 +++++++++ core/src/services/s3/backend.rs | 1 + core/src/services/s3/core.rs | 4 +++ core/src/types/capability.rs | 2 ++ core/src/types/operator/operator.rs | 30 +++++++++++++++++++++ core/src/types/operator/operator_futures.rs | 5 ++++ core/tests/behavior/async_write.rs | 20 ++++++++++++++ 7 files changed, 74 insertions(+) diff --git a/core/src/raw/ops.rs b/core/src/raw/ops.rs index 69615e0c9466..754adf1e5e2d 100644 --- a/core/src/raw/ops.rs +++ b/core/src/raw/ops.rs @@ -578,6 +578,7 @@ pub struct OpWrite { cache_control: Option, executor: Option, if_none_match: Option, + if_match: Option, if_not_exists: bool, user_metadata: Option>, } @@ -675,6 +676,17 @@ impl OpWrite { self.if_none_match.as_deref() } + /// Set the If-Match of the option + pub fn with_if_match(mut self, s: &str) -> Self { + self.if_match = Some(s.to_string()); + self + } + + /// Get If-Match from option + pub fn if_match(&self) -> Option<&str> { + self.if_match.as_deref() + } + /// Set the If-Not-Exist of the option pub fn with_if_not_exists(mut self, b: bool) -> Self { self.if_not_exists = b; diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index d509bdbc4eee..965e8008f41c 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -924,6 +924,7 @@ impl Access for S3Backend { write_can_multi: true, write_with_cache_control: true, write_with_content_type: true, + write_with_if_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 b5adcb90daa2..ad8781bcce44 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -455,6 +455,10 @@ impl S3Core { req = req.header(CACHE_CONTROL, cache_control) } + if let Some(if_match) = args.if_match() { + req = req.header(IF_MATCH, if_match); + } + if args.if_not_exists() { req = req.header(IF_NONE_MATCH, "*"); } diff --git a/core/src/types/capability.rs b/core/src/types/capability.rs index b3a9e3af9d09..a10dd9b4b250 100644 --- a/core/src/types/capability.rs +++ b/core/src/types/capability.rs @@ -130,6 +130,8 @@ pub struct Capability { pub write_with_cache_control: bool, /// Indicates if conditional write operations using If-None-Match are supported. pub write_with_if_none_match: bool, + /// Indicates if conditional write operations using If-Match are supported. + pub write_with_if_match: bool, /// Indicates if write operations can be conditional on object non-existence. pub write_with_if_not_exists: bool, /// Indicates if custom user metadata can be attached during write operations. diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index 83a7877e716e..c6343f5672f8 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1473,6 +1473,36 @@ impl Operator { /// # Ok(()) /// # } /// ``` + /// + /// ## `if_match` + /// + /// Sets an `if match` condition with specified ETag for this write request. + /// + /// ### Capability + /// + /// Check [`Capability::write_with_if_match`] before using this feature. + /// + /// ### Behavior + /// + /// - If the target file's ETag does not match the specified one, returns [`ErrorKind::ConditionNotMatch`] + /// - If the target file's ETag matches the specified one, proceeds with the write operation + /// + /// This operation will succeed when the target's ETag matches the specified one, + /// providing a way for conditional writes. + /// + /// ### Example + /// + /// ```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_match(etag).await; + /// assert!(res.is_err()); + /// assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); + /// # Ok(()) + /// # } + /// ``` pub fn write_with( &self, path: &str, diff --git a/core/src/types/operator/operator_futures.rs b/core/src/types/operator/operator_futures.rs index 53d89f594eb1..b0509254dafd 100644 --- a/core/src/types/operator/operator_futures.rs +++ b/core/src/types/operator/operator_futures.rs @@ -328,6 +328,11 @@ impl>> FutureWrite { self.map(|(args, options, bs)| (args.with_if_none_match(s), options, bs)) } + /// Set the If-Match for this operation. + pub fn if_match(self, s: &str) -> Self { + self.map(|(args, options, bs)| (args.with_if_match(s), options, bs)) + } + /// Set the If-Not-Exist for this operation. pub fn if_not_exists(self, b: bool) -> Self { self.map(|(args, options, bs)| (args.with_if_not_exists(b), options, bs)) diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index b51cc2234f38..5e0fc4ca6718 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -46,6 +46,7 @@ pub fn tests(op: &Operator, tests: &mut Vec) { test_write_with_content_disposition, test_write_with_if_none_match, test_write_with_if_not_exists, + test_write_with_if_match, test_write_with_user_metadata, test_writer_write, test_writer_write_with_overwrite, @@ -674,3 +675,22 @@ pub async fn test_write_with_if_not_exists(op: Operator) -> Result<()> { Ok(()) } + +/// Write an file with if_match will get a ConditionNotMatch error if file's etag does not match. +pub async fn test_write_with_if_match(op: Operator) -> Result<()> { + if !op.info().full_capability().write_with_if_match { + return Ok(()); + } + + let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); + + op.write(&path, content.clone()).await?; + + let meta = op.stat(&path).await?; + let etag = meta.etag().expect("etag must exist"); + + let res = op.write_with(&path, content.clone()).if_match(etag).await; + assert!(res.is_ok()); + + Ok(()) +} From d92f1b449ebbad5f75d7aad35162b6db065ac98d Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Wed, 27 Nov 2024 14:25:02 +0000 Subject: [PATCH 2/7] chore: fix comments and order --- core/src/raw/ops.rs | 24 ++++++++++++------------ core/src/types/operator/operator.rs | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/core/src/raw/ops.rs b/core/src/raw/ops.rs index 754adf1e5e2d..60a3b619ae04 100644 --- a/core/src/raw/ops.rs +++ b/core/src/raw/ops.rs @@ -577,8 +577,8 @@ pub struct OpWrite { content_disposition: Option, cache_control: Option, executor: Option, - if_none_match: Option, if_match: Option, + if_none_match: Option, if_not_exists: bool, user_metadata: Option>, } @@ -665,17 +665,6 @@ impl OpWrite { self } - /// Set the If-None-Match of the option - pub fn with_if_none_match(mut self, s: &str) -> Self { - self.if_none_match = Some(s.to_string()); - self - } - - /// Get If-None-Match from option - pub fn if_none_match(&self) -> Option<&str> { - self.if_none_match.as_deref() - } - /// Set the If-Match of the option pub fn with_if_match(mut self, s: &str) -> Self { self.if_match = Some(s.to_string()); @@ -687,6 +676,17 @@ impl OpWrite { self.if_match.as_deref() } + /// Set the If-None-Match of the option + pub fn with_if_none_match(mut self, s: &str) -> Self { + self.if_none_match = Some(s.to_string()); + self + } + + /// Get If-None-Match from option + pub fn if_none_match(&self) -> Option<&str> { + self.if_none_match.as_deref() + } + /// Set the If-Not-Exist of the option pub fn with_if_not_exists(mut self, b: bool) -> Self { self.if_not_exists = b; diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index c6343f5672f8..7c4240630049 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -1484,8 +1484,8 @@ impl Operator { /// /// ### Behavior /// - /// - If the target file's ETag does not match the specified one, returns [`ErrorKind::ConditionNotMatch`] /// - If the target file's ETag matches the specified one, proceeds with the write operation + /// - If the target file's ETag does not match the specified one, returns [`ErrorKind::ConditionNotMatch`] /// /// This operation will succeed when the target's ETag matches the specified one, /// providing a way for conditional writes. @@ -1495,9 +1495,9 @@ impl Operator { /// ```no_run /// # use opendal::{ErrorKind, Result}; /// use opendal::Operator; - /// # async fn test(op: Operator, etag: &str) -> Result<()> { + /// # async fn test(op: Operator, incorrect_etag: &str) -> Result<()> { /// let bs = b"hello, world!".to_vec(); - /// let res = op.write_with("path/to/file", bs).if_match(etag).await; + /// let res = op.write_with("path/to/file", bs).if_match(incorrect_etag).await; /// assert!(res.is_err()); /// assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); /// # Ok(()) From 32544f07779e1abf8c4886a5bcdb732c45e944b9 Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Wed, 27 Nov 2024 14:25:14 +0000 Subject: [PATCH 3/7] test: improve test --- core/tests/behavior/async_write.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 5e0fc4ca6718..348c7166c73c 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -683,8 +683,10 @@ pub async fn test_write_with_if_match(op: Operator) -> Result<()> { } let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); + let (path_incorrect, _, _) = TEST_FIXTURE.new_file(op.clone()); op.write(&path, content.clone()).await?; + op.write(&path_incorrect, content.clone()).await?; let meta = op.stat(&path).await?; let etag = meta.etag().expect("etag must exist"); @@ -692,5 +694,12 @@ pub async fn test_write_with_if_match(op: Operator) -> Result<()> { let res = op.write_with(&path, content.clone()).if_match(etag).await; assert!(res.is_ok()); + let res = op + .write_with(&path_incorrect, content.clone()) + .if_match(etag) + .await; + assert!(res.is_err()); + assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); + Ok(()) } From fd4fdd64a40419ebd70e0f3b9ae0250041b57919 Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Wed, 27 Nov 2024 14:27:03 +0000 Subject: [PATCH 4/7] chore: another order --- core/src/types/capability.rs | 4 ++-- core/src/types/operator/operator_futures.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/src/types/capability.rs b/core/src/types/capability.rs index a10dd9b4b250..405ff66e0712 100644 --- a/core/src/types/capability.rs +++ b/core/src/types/capability.rs @@ -128,10 +128,10 @@ pub struct Capability { pub write_with_content_disposition: bool, /// Indicates if Cache-Control can be specified during write operations. pub write_with_cache_control: bool, - /// Indicates if conditional write operations using If-None-Match are supported. - pub write_with_if_none_match: bool, /// Indicates if conditional write operations using If-Match are supported. pub write_with_if_match: bool, + /// Indicates if conditional write operations using If-None-Match are supported. + pub write_with_if_none_match: bool, /// Indicates if write operations can be conditional on object non-existence. pub write_with_if_not_exists: bool, /// Indicates if custom user metadata can be attached during write operations. diff --git a/core/src/types/operator/operator_futures.rs b/core/src/types/operator/operator_futures.rs index b0509254dafd..4ce296f748ff 100644 --- a/core/src/types/operator/operator_futures.rs +++ b/core/src/types/operator/operator_futures.rs @@ -323,16 +323,16 @@ impl>> FutureWrite { self.map(|(args, options, bs)| (args.with_executor(executor), options, bs)) } - /// Set the If-None-Match for this operation. - pub fn if_none_match(self, s: &str) -> Self { - self.map(|(args, options, bs)| (args.with_if_none_match(s), options, bs)) - } - /// Set the If-Match for this operation. pub fn if_match(self, s: &str) -> Self { self.map(|(args, options, bs)| (args.with_if_match(s), options, bs)) } + /// Set the If-None-Match for this operation. + pub fn if_none_match(self, s: &str) -> Self { + 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_exists(self, b: bool) -> Self { self.map(|(args, options, bs)| (args.with_if_not_exists(b), options, bs)) From 3ca070e6759b80e8968c0d6bf940568bd9c2c26c Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Wed, 27 Nov 2024 15:31:17 +0000 Subject: [PATCH 5/7] test: fix logic --- core/tests/behavior/async_write.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/core/tests/behavior/async_write.rs b/core/tests/behavior/async_write.rs index 348c7166c73c..4540925018d0 100644 --- a/core/tests/behavior/async_write.rs +++ b/core/tests/behavior/async_write.rs @@ -682,21 +682,31 @@ pub async fn test_write_with_if_match(op: Operator) -> Result<()> { return Ok(()); } - let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); - let (path_incorrect, _, _) = TEST_FIXTURE.new_file(op.clone()); + // Create two different files with different content + let (path_a, content_a, _) = TEST_FIXTURE.new_file(op.clone()); + let (path_b, content_b, _) = TEST_FIXTURE.new_file(op.clone()); - op.write(&path, content.clone()).await?; - op.write(&path_incorrect, content.clone()).await?; + // Write initial content to both files + op.write(&path_a, content_a.clone()).await?; + op.write(&path_b, content_b.clone()).await?; - let meta = op.stat(&path).await?; - let etag = meta.etag().expect("etag must exist"); + // Get etags for both files + let meta_a = op.stat(&path_a).await?; + let etag_a = meta_a.etag().expect("etag must exist"); + let meta_b = op.stat(&path_b).await?; + let etag_b = meta_b.etag().expect("etag must exist"); - let res = op.write_with(&path, content.clone()).if_match(etag).await; + // Should succeed: Writing to path_a with its own etag + let res = op + .write_with(&path_a, content_a.clone()) + .if_match(etag_a) + .await; assert!(res.is_ok()); + // Should fail: Writing to path_a with path_b's etag let res = op - .write_with(&path_incorrect, content.clone()) - .if_match(etag) + .write_with(&path_a, content_a.clone()) + .if_match(etag_b) .await; assert!(res.is_err()); assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch); From 2143099e344d872260090a0514efa06118b14ce6 Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Thu, 28 Nov 2024 16:07:27 +0000 Subject: [PATCH 6/7] feat: `disable_write_with_if_match` --- .../s3/ceph_radios_s3_with_versioning/disable_action.yml | 1 + .github/services/s3/ceph_rados_s3/action.yml | 1 + core/src/services/s3/backend.rs | 9 ++++++++- core/src/services/s3/config.rs | 4 ++++ core/src/services/s3/core.rs | 1 + core/src/services/s3/docs.md | 3 ++- 6 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/services/s3/ceph_radios_s3_with_versioning/disable_action.yml b/.github/services/s3/ceph_radios_s3_with_versioning/disable_action.yml index 7838f5f52023..71b550d91e25 100644 --- a/.github/services/s3/ceph_radios_s3_with_versioning/disable_action.yml +++ b/.github/services/s3/ceph_radios_s3_with_versioning/disable_action.yml @@ -44,4 +44,5 @@ runs: OPENDAL_S3_SECRET_ACCESS_KEY=demo OPENDAL_S3_REGION=us-east-1 OPENDAL_S3_ENABLE_VERSIONING=true + OPENDAL_S3_DISABLE_WRITE_WITH_IF_MATCH=on EOF diff --git a/.github/services/s3/ceph_rados_s3/action.yml b/.github/services/s3/ceph_rados_s3/action.yml index dfb4b5ad6a6f..54e6e7049ddb 100644 --- a/.github/services/s3/ceph_rados_s3/action.yml +++ b/.github/services/s3/ceph_rados_s3/action.yml @@ -41,4 +41,5 @@ runs: OPENDAL_S3_ACCESS_KEY_ID=demo OPENDAL_S3_SECRET_ACCESS_KEY=demo OPENDAL_S3_REGION=us-east-1 + OPENDAL_S3_DISABLE_WRITE_WITH_IF_MATCH=on EOF diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index 965e8008f41c..663bb2ff770c 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -548,6 +548,12 @@ impl S3Builder { self } + /// Disable write with if match so that opendal will not send write request with if match headers. + pub fn disable_write_with_if_match(mut self) -> Self { + self.config.disable_write_with_if_match = true; + self + } + /// Detect region of S3 bucket. /// /// # Args @@ -878,6 +884,7 @@ impl Builder for S3Builder { client, batch_max_operations, checksum_algorithm, + disable_write_with_if_match: self.config.disable_write_with_if_match, }), }) } @@ -924,7 +931,7 @@ impl Access for S3Backend { write_can_multi: true, write_with_cache_control: true, write_with_content_type: true, - write_with_if_match: true, + write_with_if_match: !self.core.disable_write_with_if_match, write_with_if_not_exists: true, write_with_user_metadata: true, diff --git a/core/src/services/s3/config.rs b/core/src/services/s3/config.rs index b1b31d5bbc72..cbcef7a207d6 100644 --- a/core/src/services/s3/config.rs +++ b/core/src/services/s3/config.rs @@ -173,6 +173,10 @@ pub struct S3Config { /// Available options: /// - "crc32c" pub checksum_algorithm: Option, + /// Disable write with if match so that opendal will not send write request with if match headers. + /// + /// For example, Ceph RADOS S3 doesn't support write with if match. + pub disable_write_with_if_match: bool, } impl Debug for S3Config { diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index ad8781bcce44..6cf689da01f9 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -98,6 +98,7 @@ pub struct S3Core { pub client: HttpClient, pub batch_max_operations: usize, pub checksum_algorithm: Option, + pub disable_write_with_if_match: bool, } impl Debug for S3Core { diff --git a/core/src/services/s3/docs.md b/core/src/services/s3/docs.md index 4027f95cac2c..2145ce58f799 100644 --- a/core/src/services/s3/docs.md +++ b/core/src/services/s3/docs.md @@ -28,8 +28,9 @@ This service can be used to: - `server_side_encryption_customer_algorithm`: Set the server_side_encryption_customer_algorithm for backend. - `server_side_encryption_customer_key`: Set the server_side_encryption_customer_key for backend. - `server_side_encryption_customer_key_md5`: Set the server_side_encryption_customer_key_md5 for backend. -- `disable_config_load`: Disable aws config load from env +- `disable_config_load`: Disable aws config load from env. - `enable_virtual_host_style`: Enable virtual host style. +- `disable_write_with_if_match`: Diable write with if match. Refer to [`S3Builder`]'s public API docs for more information. From d18ff967113e3072c743eec67be21b045fb40b20 Mon Sep 17 00:00:00 2001 From: Frank Wang <1454884738@qq.com> Date: Thu, 28 Nov 2024 16:09:07 +0000 Subject: [PATCH 7/7] chore: fix typo --- core/src/services/s3/docs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/services/s3/docs.md b/core/src/services/s3/docs.md index 2145ce58f799..1bae73dedfcd 100644 --- a/core/src/services/s3/docs.md +++ b/core/src/services/s3/docs.md @@ -30,7 +30,7 @@ This service can be used to: - `server_side_encryption_customer_key_md5`: Set the server_side_encryption_customer_key_md5 for backend. - `disable_config_load`: Disable aws config load from env. - `enable_virtual_host_style`: Enable virtual host style. -- `disable_write_with_if_match`: Diable write with if match. +- `disable_write_with_if_match`: Disable write with if match. Refer to [`S3Builder`]'s public API docs for more information.