Merge pull request 'fix(s3): report non-existent keys as deleted in bulk DeleteObjects (fix #1460)' (#1469) from smattymatty/garage:main-v2 into main-v2

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/1469
Reviewed-by: Alex <lx@deuxfleurs.fr>

src/api/s3/delete.rs

@@ -83,8 +83,21 @@ pub async fn handle_delete_objects(
 				}
 				ret_deleted.push(s3_xml::Deleted {
 					key: s3_xml::Value(obj.key.clone()),
-					version_id: s3_xml::Value(hex::encode(deleted_version)),
-					delete_marker_version_id: s3_xml::Value(hex::encode(delete_marker_version)),
+					version_id: Some(s3_xml::Value(hex::encode(deleted_version))),
+					delete_marker_version_id: Some(s3_xml::Value(hex::encode(
+						delete_marker_version,
+					))),
+				});
+			}
+			Err(Error::NoSuchKey) => {
+				if cmd.quiet {
+					continue;
+				}
+				// Deleting a non-existent key is a success in S3
+				ret_deleted.push(s3_xml::Deleted {
+					key: s3_xml::Value(obj.key.clone()),
+					version_id: None,
+					delete_marker_version_id: None,
 				});
 			}
 			Err(e) => {

src/api/s3/xml.rs

@@ -44,10 +44,13 @@ pub struct LocationConstraint {
 pub struct Deleted {
 	#[serde(rename = "Key")]
 	pub key: Value,
-	#[serde(rename = "VersionId")]
-	pub version_id: Value,
-	#[serde(rename = "DeleteMarkerVersionId")]
-	pub delete_marker_version_id: Value,
+	#[serde(rename = "VersionId", skip_serializing_if = "Option::is_none")]
+	pub version_id: Option<Value>,
+	#[serde(
+		rename = "DeleteMarkerVersionId",
+		skip_serializing_if = "Option::is_none"
+	)]
+	pub delete_marker_version_id: Option<Value>,
 }
 
 #[derive(Debug, Serialize, PartialEq, Eq)]
@@ -497,13 +500,13 @@ mod tests {
 			deleted: vec![
 				Deleted {
 					key: Value("a/plop".to_string()),
-					version_id: Value("qsdfjklm".to_string()),
-					delete_marker_version_id: Value("wxcvbn".to_string()),
+					version_id: Some(Value("qsdfjklm".to_string())),
+					delete_marker_version_id: Some(Value("wxcvbn".to_string())),
 				},
 				Deleted {
 					key: Value("b/plip".to_string()),
-					version_id: Value("1234".to_string()),
-					delete_marker_version_id: Value("4321".to_string()),
+					version_id: Some(Value("1234".to_string())),
+					delete_marker_version_id: Some(Value("4321".to_string())),
 				},
 			],
 			errors: vec![

src/garage/tests/s3/objects.rs

@@ -554,7 +554,7 @@ async fn test_deleteobject() {
 
 	assert!(l.contents.is_none());
 
-	// Deleting a non-existing object shouldn't be a problem
+	// Deleting a non-existing object shouldn't be a problem...
 	ctx.client
 		.delete_object()
 		.bucket(&bucket)
@@ -562,4 +562,27 @@ async fn test_deleteobject() {
 		.send()
 		.await
 		.unwrap();
+
+	// ...and bulk-deleting a non-existing object shouldn't be either
+	let r = ctx
+		.client
+		.delete_objects()
+		.bucket(&bucket)
+		.delete(
+			Delete::builder()
+				.objects(
+					ObjectIdentifier::builder()
+						.key("does-not-exist")
+						.build()
+						.unwrap(),
+				)
+				.build()
+				.unwrap(),
+		)
+		.send()
+		.await
+		.unwrap();
+
+	assert_eq!(r.deleted.unwrap().len(), 1); // reported as deleted...
+	assert!(r.errors.unwrap_or_default().is_empty()); // ...not as an error
 }