diff --git a/.github/services/s3/0_minio_s3/action.yml b/.github/services/s3/0_minio_s3/action.yml index f74a07701c56..390e0c9296dc 100644 --- a/.github/services/s3/0_minio_s3/action.yml +++ b/.github/services/s3/0_minio_s3/action.yml @@ -43,5 +43,5 @@ runs: OPENDAL_S3_ACCESS_KEY_ID=minioadmin OPENDAL_S3_SECRET_ACCESS_KEY=minioadmin OPENDAL_S3_REGION=us-east-1 - OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false + OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false,delete_with_if_match=false EOF diff --git a/.github/services/s3/minio_s3_with_anonymous/action.yml b/.github/services/s3/minio_s3_with_anonymous/action.yml index 5bd46d42f2c0..fc0ee27a5756 100644 --- a/.github/services/s3/minio_s3_with_anonymous/action.yml +++ b/.github/services/s3/minio_s3_with_anonymous/action.yml @@ -49,5 +49,5 @@ runs: OPENDAL_S3_REGION=us-east-1 OPENDAL_S3_ALLOW_ANONYMOUS=on OPENDAL_S3_DISABLE_EC2_METADATA=on - OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false + OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false,delete_with_if_match=false EOF diff --git a/.github/services/s3/minio_s3_with_list_objects_v1/action.yml b/.github/services/s3/minio_s3_with_list_objects_v1/action.yml index af0e197a4980..11c6c2855dcb 100644 --- a/.github/services/s3/minio_s3_with_list_objects_v1/action.yml +++ b/.github/services/s3/minio_s3_with_list_objects_v1/action.yml @@ -44,5 +44,5 @@ runs: OPENDAL_S3_SECRET_ACCESS_KEY=minioadmin OPENDAL_S3_REGION=us-east-1 OPENDAL_S3_DISABLE_LIST_OBJECTS_V2=true - OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false + OPENDAL_TEST_CAPABILITY_OVERRIDES=stat_with_version=false,read_with_version=false,delete_with_version=false,list_with_versions=false,list_with_deleted=false,copy_with_source_version=false,write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false,delete_with_if_match=false EOF diff --git a/.github/services/s3/minio_s3_with_versioning/action.yml b/.github/services/s3/minio_s3_with_versioning/action.yml index 361bb057649d..f3997717b1a5 100644 --- a/.github/services/s3/minio_s3_with_versioning/action.yml +++ b/.github/services/s3/minio_s3_with_versioning/action.yml @@ -44,5 +44,5 @@ runs: OPENDAL_S3_ACCESS_KEY_ID=minioadmin OPENDAL_S3_SECRET_ACCESS_KEY=minioadmin OPENDAL_S3_REGION=us-east-1 - OPENDAL_TEST_CAPABILITY_OVERRIDES=write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false + OPENDAL_TEST_CAPABILITY_OVERRIDES=write_can_append=false,copy_with_if_not_exists=false,copy_with_if_match=false,delete_with_if_match=false EOF diff --git a/bindings/c/src/operator.rs b/bindings/c/src/operator.rs index 6b5b2f2ecbad..87ffaa1f0a72 100644 --- a/bindings/c/src/operator.rs +++ b/bindings/c/src/operator.rs @@ -682,6 +682,7 @@ pub unsafe extern "C" fn opendal_operator_delete_with( core::options::DeleteOptions { version, recursive: o.recursive, + if_match: None, } }; match op.deref().delete_options(path, delete_opts) { diff --git a/bindings/nodejs/generated.d.ts b/bindings/nodejs/generated.d.ts index 8ac5619d1bac..cdaeac778bd6 100644 --- a/bindings/nodejs/generated.d.ts +++ b/bindings/nodejs/generated.d.ts @@ -1101,6 +1101,11 @@ export interface DeleteOptions { version?: string /** Whether to delete recursively. */ recursive?: boolean + /** * Sets if-match condition for this operation. + * If file exists and its etag does not match, an error of kind + * `ConditionNotMatch` will be returned. + */ + ifMatch?: string } export declare const enum EntryMode { diff --git a/bindings/nodejs/src/options.rs b/bindings/nodejs/src/options.rs index ccb1cae5b020..43b4d8fefeb3 100644 --- a/bindings/nodejs/src/options.rs +++ b/bindings/nodejs/src/options.rs @@ -512,6 +512,12 @@ pub struct DeleteOptions { pub version: Option, /// Whether to delete recursively. pub recursive: Option, + /** + * Sets if-match condition for this operation. + * If file exists and its etag does not match, an error of kind + * `ConditionNotMatch` will be returned. + */ + pub if_match: Option, } impl From for opendal::options::DeleteOptions { @@ -519,6 +525,7 @@ impl From for opendal::options::DeleteOptions { Self { version: value.version, recursive: value.recursive.unwrap_or_default(), + if_match: value.if_match, } } } diff --git a/bindings/python/python/opendal/operator.pyi b/bindings/python/python/opendal/operator.pyi index 8ecb4dd2cadd..9831a7281ceb 100644 --- a/bindings/python/python/opendal/operator.pyi +++ b/bindings/python/python/opendal/operator.pyi @@ -340,6 +340,7 @@ class AsyncOperator: *, version: builtins.str | None = None, recursive: builtins.bool | None = None, + if_match: builtins.str | None = None, ) -> collections.abc.Awaitable[None]: r""" Delete a file at the given path. @@ -3069,6 +3070,7 @@ class Operator: *, version: builtins.str | None = None, recursive: builtins.bool | None = None, + if_match: builtins.str | None = None, ) -> None: r""" Delete a file at the given path. diff --git a/bindings/python/src/operator.rs b/bindings/python/src/operator.rs index af45e25a962b..16491087955f 100644 --- a/bindings/python/src/operator.rs +++ b/bindings/python/src/operator.rs @@ -530,18 +530,22 @@ impl Operator { /// The version of the file to delete. Only supported on version-aware backends. /// recursive : bool, optional /// If True, delete the path recursively. Only supported on backends that support recursive delete. - #[pyo3(signature = (path, *, version=None, recursive=None))] + /// if_match : str, optional + /// If set, the delete will only succeed when the existing object's ETag matches. + #[pyo3(signature = (path, *, version=None, recursive=None, if_match=None))] pub fn delete( &self, path: PathBuf, version: Option, recursive: Option, + if_match: Option, ) -> PyResult<()> { let path = path.to_string_lossy().to_string(); - if version.is_some() || recursive.is_some() { + if version.is_some() || recursive.is_some() || if_match.is_some() { let opts = ocore::options::DeleteOptions { version, recursive: recursive.unwrap_or(false), + if_match, }; self.core.delete_options(&path, opts).map_err(format_pyerr) } else { @@ -1330,21 +1334,25 @@ impl AsyncOperator { /// The version of the file to delete. Only supported on version-aware backends. /// recursive : bool, optional /// If True, delete the path recursively. Only supported on backends that support recursive delete. - #[pyo3(signature = (path, *, version=None, recursive=None))] + /// if_match : str, optional + /// If set, the delete will only succeed when the existing object's ETag matches. + #[pyo3(signature = (path, *, version=None, recursive=None, if_match=None))] pub fn delete<'p>( &'p self, py: Python<'p>, path: PathBuf, version: Option, recursive: Option, + if_match: Option, ) -> PyResult> { let this = self.core.clone(); let path = path.to_string_lossy().to_string(); future_into_py(py, async move { - if version.is_some() || recursive.is_some() { + if version.is_some() || recursive.is_some() || if_match.is_some() { let opts = ocore::options::DeleteOptions { version, recursive: recursive.unwrap_or(false), + if_match, }; this.delete_options(&path, opts).await.map_err(format_pyerr) } else { diff --git a/bindings/python/src/options.rs b/bindings/python/src/options.rs index 1d37e0e7112e..3909c1d8d916 100644 --- a/bindings/python/src/options.rs +++ b/bindings/python/src/options.rs @@ -289,6 +289,7 @@ impl From for ocore::options::StatOptions { pub struct DeleteOptions { pub version: Option, pub recursive: Option, + pub if_match: Option, } impl<'a, 'py> FromPyObject<'a, 'py> for DeleteOptions { @@ -300,6 +301,7 @@ impl<'a, 'py> FromPyObject<'a, 'py> for DeleteOptions { Ok(Self { version: extract_optional(&dict, "version")?, recursive: extract_optional(&dict, "recursive")?, + if_match: extract_optional(&dict, "if_match")?, }) } } @@ -309,6 +311,7 @@ impl From for ocore::options::DeleteOptions { Self { version: opts.version, recursive: opts.recursive.unwrap_or(false), + if_match: opts.if_match, } } } diff --git a/core/core/src/layers/correctness_check.rs b/core/core/src/layers/correctness_check.rs index 5db06b7d81b3..94384ed6646a 100644 --- a/core/core/src/layers/correctness_check.rs +++ b/core/core/src/layers/correctness_check.rs @@ -283,6 +283,14 @@ impl CheckWrapper { )); } + if args.if_match().is_some() && !self.info.full_capability().delete_with_if_match { + return Err(new_unsupported_error( + &self.info, + Operation::Delete, + "if_match", + )); + } + Ok(()) } } diff --git a/core/core/src/raw/ops.rs b/core/core/src/raw/ops.rs index aa5abc9468c8..c15d70d6c32d 100644 --- a/core/core/src/raw/ops.rs +++ b/core/core/src/raw/ops.rs @@ -44,6 +44,7 @@ impl OpCreateDir { pub struct OpDelete { version: Option, recursive: bool, + if_match: Option, } impl OpDelete { @@ -66,6 +67,15 @@ impl OpDelete { self } + /// Set the if_match condition for this delete operation. + /// + /// When set, the delete will only proceed if the existing object's ETag + /// matches the given value. + pub fn with_if_match(mut self, if_match: impl Into) -> Self { + self.if_match = Some(if_match.into()); + self + } + /// Get the version of this delete operation. pub fn version(&self) -> Option<&str> { self.version.as_deref() @@ -75,6 +85,11 @@ impl OpDelete { pub fn recursive(&self) -> bool { self.recursive } + + /// Get the if_match condition. + pub fn if_match(&self) -> Option<&str> { + self.if_match.as_deref() + } } impl From for OpDelete { @@ -82,6 +97,7 @@ impl From for OpDelete { Self { version: value.version, recursive: value.recursive, + if_match: value.if_match, } } } diff --git a/core/core/src/types/capability.rs b/core/core/src/types/capability.rs index ac5148452f2c..8f488c1f815a 100644 --- a/core/core/src/types/capability.rs +++ b/core/core/src/types/capability.rs @@ -149,6 +149,8 @@ pub struct Capability { pub delete_with_version: bool, /// Indicates if recursive delete operations are supported. pub delete_with_recursive: bool, + /// Indicates if conditional delete operations using If-Match are supported. + pub delete_with_if_match: bool, /// Maximum size supported for single delete operations. pub delete_max_size: Option, diff --git a/core/core/src/types/operator/operator_futures.rs b/core/core/src/types/operator/operator_futures.rs index 6392366a45d9..9a413c569b52 100644 --- a/core/core/src/types/operator/operator_futures.rs +++ b/core/core/src/types/operator/operator_futures.rs @@ -1279,6 +1279,12 @@ impl>> FutureDelete { self.args.recursive = recursive; self } + + /// Set `if_match` for this delete operation. + pub fn if_match(mut self, etag: &str) -> Self { + self.args.if_match = Some(etag.to_string()); + self + } } /// Future that generated by [`Operator::deleter_with`]. diff --git a/core/core/src/types/options.rs b/core/core/src/types/options.rs index 1435c37f0b08..5e7dddf3ee93 100644 --- a/core/core/src/types/options.rs +++ b/core/core/src/types/options.rs @@ -31,6 +31,18 @@ pub struct DeleteOptions { /// - If `true`, all entries under the path (or sharing the prefix for file-like paths) /// will be removed. pub recursive: bool, + /// Sets the condition that delete will succeed only if the existing + /// object has the given ETag. + /// + /// ### Capability + /// + /// Check [`Capability::delete_with_if_match`] before using this feature. + /// + /// ### Behavior + /// + /// - If supported, the delete will only succeed when the existing object's + /// ETag matches the given value. + pub if_match: Option, } /// Options for list operations. diff --git a/core/services/s3/src/backend.rs b/core/services/s3/src/backend.rs index dc29f6284fcf..d125efbd8974 100644 --- a/core/services/s3/src/backend.rs +++ b/core/services/s3/src/backend.rs @@ -978,6 +978,7 @@ impl Builder for S3Builder { delete: true, delete_max_size: Some(DEFAULT_BATCH_MAX_OPERATIONS), delete_with_version: true, + delete_with_if_match: true, copy: true, copy_can_multi: true, diff --git a/core/services/s3/src/core.rs b/core/services/s3/src/core.rs index c08df46b6df6..80e6f5936fa9 100644 --- a/core/services/s3/src/core.rs +++ b/core/services/s3/src/core.rs @@ -641,6 +641,11 @@ impl S3Core { let mut req = Request::delete(&url); + // Set conditional delete header. + if let Some(if_match) = args.if_match() { + req = req.header(IF_MATCH, if_match); + } + // Set request payer header if enabled. req = self.insert_request_payer_header(req); @@ -1232,6 +1237,7 @@ impl S3Core { .map(|(path, op)| DeleteObjectsRequestObject { key: build_abs_path(&self.root, path), version_id: op.version().map(|v| v.to_owned()), + etag: op.if_match().map(|v| v.to_owned()), }) .collect(), }) @@ -1406,6 +1412,8 @@ pub struct DeleteObjectsRequestObject { pub key: String, #[serde(skip_serializing_if = "Option::is_none")] pub version_id: Option, + #[serde(rename = "ETag", skip_serializing_if = "Option::is_none")] + pub etag: Option, } /// Result of DeleteObjects. @@ -1668,10 +1676,17 @@ mod tests { DeleteObjectsRequestObject { key: "sample1.txt".to_string(), version_id: None, + etag: None, }, DeleteObjectsRequestObject { key: "sample2.txt".to_string(), version_id: Some("11111".to_owned()), + etag: None, + }, + DeleteObjectsRequestObject { + key: "sample3.txt".to_string(), + version_id: None, + etag: Some("\"d41d8cd98f00b204e9800998ecf8427e\"".to_owned()), }, ], }; @@ -1689,6 +1704,10 @@ mod tests { sample2.txt 11111 + + sample3.txt + "d41d8cd98f00b204e9800998ecf8427e" + "# // Cleanup space and new line .replace([' ', '\n'], "") diff --git a/core/tests/behavior/async_delete.rs b/core/tests/behavior/async_delete.rs index 3d1893a7cd46..83800f8642f2 100644 --- a/core/tests/behavior/async_delete.rs +++ b/core/tests/behavior/async_delete.rs @@ -48,6 +48,13 @@ pub fn tests(op: &Operator, tests: &mut Vec) { tests.extend(async_trials!(op, test_remove_all_with_prefix_exists)); } } + if cap.delete_with_if_match { + tests.extend(async_trials!( + op, + test_delete_with_if_match_match, + test_delete_with_if_match_mismatch + )); + } } } @@ -414,3 +421,44 @@ pub async fn test_batch_delete_with_version(op: Operator) -> Result<()> { Ok(()) } + +/// Delete with a matching `If-Match` ETag should succeed and remove the object. +pub async fn test_delete_with_if_match_match(op: Operator) -> Result<()> { + if !op.info().full_capability().delete_with_if_match { + return Ok(()); + } + + let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); + op.write(&path, content).await.expect("write must succeed"); + + let meta = op.stat(&path).await.expect("stat must succeed"); + let etag = meta.etag().expect("etag must be present"); + + op.delete_with(&path).if_match(etag).await?; + + assert!(!op.exists(&path).await?); + + Ok(()) +} + +/// Delete with a non-matching `If-Match` ETag should fail with +/// [`ErrorKind::ConditionNotMatch`] and leave the object intact. +pub async fn test_delete_with_if_match_mismatch(op: Operator) -> Result<()> { + if !op.info().full_capability().delete_with_if_match { + return Ok(()); + } + + let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); + op.write(&path, content).await.expect("write must succeed"); + + let err = op + .delete_with(&path) + .if_match("\"this-etag-does-not-match\"") + .await + .expect_err("delete must fail when etag mismatches"); + assert_eq!(err.kind(), ErrorKind::ConditionNotMatch); + + assert!(op.exists(&path).await?); + + Ok(()) +}