feat(services/s3): add conditional delete with if-match (#7607)
* feat(services/s3): add conditional delete with if-match (closes #7090)
* fix: resolve conditional delete CI failures
---------
Co-authored-by: Xuanwo <github@xuanwo.io>
diff --git a/.github/services/s3/0_minio_s3/action.yml b/.github/services/s3/0_minio_s3/action.yml
index f74a077..390e0c9 100644
--- a/.github/services/s3/0_minio_s3/action.yml
+++ b/.github/services/s3/0_minio_s3/action.yml
@@ -43,5 +43,5 @@
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 5bd46d4..fc0ee27 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 @@
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 af0e197..11c6c28 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 @@
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 361bb05..f399771 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 @@
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 01b92a7..0a9d77b 100644
--- a/bindings/c/src/operator.rs
+++ b/bindings/c/src/operator.rs
@@ -819,6 +819,7 @@
core::options::DeleteOptions {
version,
recursive: o.recursive,
+ if_match: None,
}
};
match op.deref().delete_options(path, delete_opts) {
diff --git a/bindings/c/src/presign.rs b/bindings/c/src/presign.rs
index 43ce897..4b18cbe 100644
--- a/bindings/c/src/presign.rs
+++ b/bindings/c/src/presign.rs
@@ -241,6 +241,7 @@
core::options::DeleteOptions {
version,
recursive: opts.recursive,
+ if_match: None,
}
};
diff --git a/bindings/cpp/src/lib.rs b/bindings/cpp/src/lib.rs
index e0651b2..0a06516 100644
--- a/bindings/cpp/src/lib.rs
+++ b/bindings/cpp/src/lib.rs
@@ -535,6 +535,7 @@
od::options::DeleteOptions {
version: optional_string(opts.version),
recursive: opts.recursive,
+ if_match: None,
}
}
diff --git a/bindings/dotnet/src/options.rs b/bindings/dotnet/src/options.rs
index 0a84e0e..d0672f4 100644
--- a/bindings/dotnet/src/options.rs
+++ b/bindings/dotnet/src/options.rs
@@ -217,5 +217,6 @@
Ok(opendal::options::DeleteOptions {
version: parse_string(values, "version"),
recursive: parse_bool(values, "recursive")?.unwrap_or(false),
+ if_match: None,
})
}
diff --git a/bindings/nodejs/generated.d.ts b/bindings/nodejs/generated.d.ts
index 8ac5619..cdaeac7 100644
--- a/bindings/nodejs/generated.d.ts
+++ b/bindings/nodejs/generated.d.ts
@@ -1101,6 +1101,11 @@
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 31a5860..592f170 100644
--- a/bindings/nodejs/src/options.rs
+++ b/bindings/nodejs/src/options.rs
@@ -513,6 +513,12 @@
pub version: Option<String>,
/// Whether to delete recursively.
pub recursive: Option<bool>,
+ /**
+ * 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<String>,
}
impl From<DeleteOptions> for opendal::options::DeleteOptions {
@@ -520,6 +526,7 @@
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 7062a21..4fddebd 100644
--- a/bindings/python/python/opendal/operator.pyi
+++ b/bindings/python/python/opendal/operator.pyi
@@ -128,6 +128,7 @@
*,
version: str | None = None,
recursive: bool | None = None,
+ if_match: str | None = None,
) -> collections.abc.Awaitable[None]:
"""
Delete a file at the given path.
@@ -150,6 +151,8 @@
recursive : bool, optional
If True, delete the path recursively.
Only supported on backends that support recursive delete.
+ if_match : str, optional
+ If set, only delete when the existing object's ETag matches.
"""
def exists(self, /, path: str | PathLike[str]) -> collections.abc.Awaitable[bool]:
"""
@@ -786,6 +789,7 @@
*,
version: str | None = None,
recursive: bool | None = None,
+ if_match: str | None = None,
) -> None:
"""
Delete a file at the given path.
@@ -803,6 +807,8 @@
recursive : bool, optional
If True, delete the path recursively.
Only supported on backends that support recursive delete.
+ if_match : str, optional
+ If set, only delete when the existing object's ETag matches.
"""
def exists(self, /, path: str | PathLike[str]) -> bool:
"""
diff --git a/bindings/python/python/opendal/options.py b/bindings/python/python/opendal/options.py
index 2011ed2..2862fea 100644
--- a/bindings/python/python/opendal/options.py
+++ b/bindings/python/python/opendal/options.py
@@ -152,6 +152,8 @@
Only supported on backends that support recursive delete.
"""
+ if_match: str
+ """The ETag that the object must match before deletion."""
class OpenKwargs(ReadKwargs, WriteKwargs, total=False):
diff --git a/bindings/python/src/operator.rs b/bindings/python/src/operator.rs
index 3d11d85..47de79e 100644
--- a/bindings/python/src/operator.rs
+++ b/bindings/python/src/operator.rs
@@ -709,18 +709,22 @@
/// 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, only delete 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<String>,
recursive: Option<bool>,
+ if_match: Option<String>,
) -> 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 {
@@ -1530,21 +1534,25 @@
/// recursive : bool, optional
/// If True, delete the path recursively.
/// Only supported on backends that support recursive delete.
- #[pyo3(signature = (path, *, version=None, recursive=None) -> "collections.abc.Awaitable[None]")]
+ /// if_match : str, optional
+ /// If set, only delete when the existing object's ETag matches.
+ #[pyo3(signature = (path, *, version=None, recursive=None, if_match=None) -> "collections.abc.Awaitable[None]")]
pub fn delete<'p>(
&'p self,
py: Python<'p>,
path: PathBuf,
version: Option<String>,
recursive: Option<bool>,
+ if_match: Option<String>,
) -> PyResult<Bound<'p, PyAny>> {
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 1a578d5..0102502 100644
--- a/bindings/python/src/options.rs
+++ b/bindings/python/src/options.rs
@@ -336,6 +336,8 @@
///
/// Only supported on backends that support recursive delete.
pub recursive: Option<bool>,
+ /// The ETag that the object must match before deletion.
+ pub if_match: Option<String>,
}
impl<'a, 'py> FromPyObject<'a, 'py> for DeleteOptions {
@@ -347,6 +349,7 @@
Ok(Self {
version: extract_optional(&dict, "version")?,
recursive: extract_optional(&dict, "recursive")?,
+ if_match: extract_optional(&dict, "if_match")?,
})
}
}
@@ -356,6 +359,7 @@
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 f10ef96..603cfd7 100644
--- a/core/core/src/layers/correctness_check.rs
+++ b/core/core/src/layers/correctness_check.rs
@@ -303,6 +303,14 @@
));
}
+ if args.if_match().is_some() && !self.capability.delete_with_if_match {
+ return Err(new_unsupported_error(
+ self.scheme,
+ Operation::Delete,
+ "if_match",
+ ));
+ }
+
Ok(())
}
}
diff --git a/core/core/src/raw/ops.rs b/core/core/src/raw/ops.rs
index 99cdd2d..7f32031 100644
--- a/core/core/src/raw/ops.rs
+++ b/core/core/src/raw/ops.rs
@@ -48,6 +48,9 @@
/// Whether a `delete` is recursive.
recursive: bool,
+
+ /// The ETag that the object must match before deletion.
+ if_match: Option<String>,
}
impl OpDelete {
@@ -70,6 +73,14 @@
self
}
+ /// Set the ETag that the object must match before deletion.
+ ///
+ /// The operation fails when the existing object's ETag does not match.
+ pub fn with_if_match(mut self, if_match: impl Into<String>) -> Self {
+ self.if_match = Some(if_match.into());
+ self
+ }
+
/// Return the version of the object to delete.
pub fn version(&self) -> Option<&str> {
self.version.as_deref()
@@ -79,6 +90,11 @@
pub fn recursive(&self) -> bool {
self.recursive
}
+
+ /// Return the ETag that the object must match before deletion.
+ pub fn if_match(&self) -> Option<&str> {
+ self.if_match.as_deref()
+ }
}
impl From<options::DeleteOptions> for OpDelete {
@@ -86,6 +102,7 @@
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 9994f6e..cca00a6 100644
--- a/core/core/src/types/capability.rs
+++ b/core/core/src/types/capability.rs
@@ -144,6 +144,8 @@
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<usize>,
diff --git a/core/core/src/types/operator/operator_futures.rs b/core/core/src/types/operator/operator_futures.rs
index d5353dd..b8b6c42 100644
--- a/core/core/src/types/operator/operator_futures.rs
+++ b/core/core/src/types/operator/operator_futures.rs
@@ -1284,6 +1284,12 @@
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
+ }
}
/// This future creates a [`Deleter`].
diff --git a/core/core/src/types/options.rs b/core/core/src/types/options.rs
index 5972183..15c8aa3 100644
--- a/core/core/src/types/options.rs
+++ b/core/core/src/types/options.rs
@@ -32,6 +32,18 @@
/// - 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 [`crate::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<String>,
}
/// Options for list operations.
diff --git a/core/services/s3/src/backend.rs b/core/services/s3/src/backend.rs
index 160e737..33081cc 100644
--- a/core/services/s3/src/backend.rs
+++ b/core/services/s3/src/backend.rs
@@ -996,6 +996,7 @@
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 a2278a1..0aee67f 100644
--- a/core/services/s3/src/core.rs
+++ b/core/services/s3/src/core.rs
@@ -667,6 +667,11 @@
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);
@@ -1284,6 +1289,7 @@
.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(),
})
@@ -1459,6 +1465,8 @@
pub key: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub version_id: Option<String>,
+ #[serde(rename = "ETag", skip_serializing_if = "Option::is_none")]
+ pub etag: Option<String>,
}
/// Result of DeleteObjects.
@@ -1721,10 +1729,17 @@
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()),
},
],
};
@@ -1742,6 +1757,10 @@
<Key>sample2.txt</Key>
<VersionId>11111</VersionId>
</Object>
+ <Object>
+ <Key>sample3.txt</Key>
+ <ETag>"d41d8cd98f00b204e9800998ecf8427e"</ETag>
+ </Object>
</Delete>"#
// Cleanup space and new line
.replace([' ', '\n'], "")
diff --git a/core/tests/behavior/async_delete.rs b/core/tests/behavior/async_delete.rs
index ae441cc..9effe5d 100644
--- a/core/tests/behavior/async_delete.rs
+++ b/core/tests/behavior/async_delete.rs
@@ -48,6 +48,13 @@
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 @@
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().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().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(())
+}
diff --git a/dev/src/generate/options.rs b/dev/src/generate/options.rs
index 16c7031..e513560 100644
--- a/dev/src/generate/options.rs
+++ b/dev/src/generate/options.rs
@@ -220,10 +220,16 @@
);
let delete = &options[4];
- assert_eq!(delete.fields.len(), 2);
+ assert_eq!(delete.fields.len(), 3);
assert_eq!(
delete.fields[1].doc,
"If True, delete the path recursively.\n\nOnly supported on backends that support recursive delete."
);
+ assert_eq!(delete.fields[2].name, "if_match");
+ assert_eq!(delete.fields[2].ty, OptionType::Str);
+ assert_eq!(
+ delete.fields[2].doc,
+ "The ETag that the object must match before deletion."
+ );
}
}
