feat(rm): add purge support for force delete#82
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes RustFS’s force-delete capability in the CLI by adding rm --purge, and wires it through the S3 adapter so delete requests can include the x-rustfs-force-delete: true header when requested.
Changes:
- Add
rm --purgeflag and pass purge intent down to the S3 layer viaDeleteRequestOptions. - Add purge-aware delete helpers to
rc-s3that conditionally attach the RustFS force-delete header for single and multi-object deletes. - Add unit tests for request header behavior and an integration test validating versioned delete-marker vs permanent-delete behavior.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/s3/src/lib.rs | Re-exports DeleteRequestOptions for CLI consumption. |
| crates/s3/src/client.rs | Introduces purge-aware delete helpers, routes trait delete calls through them, and adds request-capture unit tests. |
| crates/s3/Cargo.toml | Adds a new dev-dependency for Smithy HTTP request capture utilities. |
| crates/cli/src/commands/rm.rs | Adds --purge flag and uses purge-aware delete helpers for single and recursive deletes. |
| crates/cli/tests/integration.rs | Adds an integration test verifying delete markers vs permanent deletion under versioning. |
| Cargo.lock | Locks new dev-dependency transitive graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut request = self | ||
| .inner | ||
| .delete_object() | ||
| .bucket(&path.bucket) | ||
| .key(&path.key) | ||
| .customize(); | ||
|
|
||
| if options.force_delete { | ||
| request = request.mutate_request(|request| { | ||
| request | ||
| .headers_mut() | ||
| .insert(RUSTFS_FORCE_DELETE_HEADER, "true"); | ||
| }); | ||
| } | ||
|
|
||
| request.send().await.map_err(|e| { |
There was a problem hiding this comment.
delete_object_with_options always builds a customized request (.customize()), even when force_delete is false. This introduces avoidable overhead for the common path (normal deletes). Consider only calling .customize()/mutate_request() when options.force_delete is true, and otherwise using the non-customized .send() path (as before).
| let mut request = self | |
| .inner | |
| .delete_object() | |
| .bucket(&path.bucket) | |
| .key(&path.key) | |
| .customize(); | |
| if options.force_delete { | |
| request = request.mutate_request(|request| { | |
| request | |
| .headers_mut() | |
| .insert(RUSTFS_FORCE_DELETE_HEADER, "true"); | |
| }); | |
| } | |
| request.send().await.map_err(|e| { | |
| let request = self | |
| .inner | |
| .delete_object() | |
| .bucket(&path.bucket) | |
| .key(&path.key); | |
| let result = if options.force_delete { | |
| request | |
| .customize() | |
| .mutate_request(|request| { | |
| request | |
| .headers_mut() | |
| .insert(RUSTFS_FORCE_DELETE_HEADER, "true"); | |
| }) | |
| .send() | |
| .await | |
| } else { | |
| request.send().await | |
| }; | |
| result.map_err(|e| { |
| request.send().await.map_err(|e| { | ||
| let err_str = e.to_string(); | ||
| if err_str.contains("NotFound") || err_str.contains("NoSuchKey") { | ||
| Error::NotFound(path.to_string()) | ||
| } else { | ||
| Error::Network(err_str) | ||
| } | ||
| })?; |
There was a problem hiding this comment.
delete_object_with_options classifies NotFound/NoSuchKey by substring-matching e.to_string(). This is brittle (message formats can change) and may misclassify errors. Prefer inspecting structured metadata (e.g., service error code / HTTP status via ProvideErrorMetadata / raw().http().status() on SdkError::ServiceError) to map NotFound reliably.
| keys.iter() | ||
| .map(|key| { | ||
| ObjectIdentifier::builder().key(key).build().map_err(|e| { | ||
| Error::General(format!("invalid delete object identifier: {e}")) |
There was a problem hiding this comment.
When building ObjectIdentifiers, the error message omits which key failed. Including the offending key in the invalid delete object identifier error would make debugging malformed keys much easier.
| Error::General(format!("invalid delete object identifier: {e}")) | |
| Error::General(format!( | |
| "invalid delete object identifier for key {:?}: {e}", | |
| key | |
| )) |
| let error_keys: Vec<String> = response | ||
| .errors() | ||
| .iter() | ||
| .filter_map(|e| e.key().map(|k| k.to_string())) | ||
| .collect(); | ||
| tracing::warn!("Failed to delete some objects: {:?}", error_keys); |
There was a problem hiding this comment.
delete_objects_with_options returns Ok(deleted) even when response.errors() is non-empty, only emitting a log warning. As a public helper, this can mislead callers into treating partial deletion as success. Consider returning an error (or a richer result type including both deleted + errors) so callers can handle partial failures deterministically.
| let error_keys: Vec<String> = response | |
| .errors() | |
| .iter() | |
| .filter_map(|e| e.key().map(|k| k.to_string())) | |
| .collect(); | |
| tracing::warn!("Failed to delete some objects: {:?}", error_keys); | |
| let error_details: Vec<String> = response | |
| .errors() | |
| .iter() | |
| .map(|e| { | |
| let key = e.key().unwrap_or("<unknown>"); | |
| match (e.code(), e.message()) { | |
| (Some(code), Some(message)) => { | |
| format!("{key} ({code}: {message})") | |
| } | |
| (Some(code), None) => format!("{key} ({code})"), | |
| (None, Some(message)) => format!("{key} ({message})"), | |
| (None, None) => key.to_string(), | |
| } | |
| }) | |
| .collect(); | |
| tracing::warn!("Failed to delete some objects: {:?}", error_details); | |
| return Err(Error::General(format!( | |
| "failed to delete some objects: {}", | |
| error_details.join(", ") | |
| ))); |
| quick-xml.workspace = true | ||
|
|
||
| [dev-dependencies] | ||
| aws-smithy-http-client = { version = "1.1.5", features = ["test-util"] } |
There was a problem hiding this comment.
This crate appears to use workspace-managed dependency versions (e.g., other dev-dependencies are *.workspace = true). Adding aws-smithy-http-client with an inline version here can lead to version skew across crates. Consider adding it to [workspace.dependencies] in the root Cargo.toml and referencing it here via aws-smithy-http-client.workspace = true (keeping the test-util feature).
| aws-smithy-http-client = { version = "1.1.5", features = ["test-util"] } | |
| aws-smithy-http-client = { workspace = true, features = ["test-util"] } |
| let request = request_receiver.expect_request(); | ||
| assert_eq!(request.headers().get("x-rustfs-force-delete"), Some("true")); | ||
| } |
There was a problem hiding this comment.
The tests hard-code the header name string ("x-rustfs-force-delete") instead of using RUSTFS_FORCE_DELETE_HEADER. Using the constant would prevent the tests from silently drifting if the header name changes.
Summary
rm --purgeflag that sends the RustFS force-delete header for single-object and recursive deletes--purgepermanently removes versioned objectsBackground
RustFS already supports permanent deletion through the
x-rustfs-force-delete: truerequest header, but the CLI did not expose that capability. As a result, versioned deletes always produced delete markers even when the backend could perform a permanent delete.Validation
cargo fmt --allcargo clippy --workspace -- -D warningscargo test --workspaceTEST_S3_ENDPOINT=http://localhost:9000 TEST_S3_ACCESS_KEY=accesskey TEST_S3_SECRET_KEY=secretkey cargo test --package rustfs-cli --test integration --features integration version_operations::test_rm_purge_permanently_deletes_versioned_object -- --exact --nocapture