From 77eb91bf8d57cc57eb9e6748f31bcee95379c212 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 9 Jul 2026 01:07:06 +0800 Subject: [PATCH] fix(s3-types): cover all removed events and fix scanner round-trip (#4512) --- crates/s3-ops/src/lib.rs | 14 ++++- crates/s3-types/src/event_name.rs | 96 +++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/crates/s3-ops/src/lib.rs b/crates/s3-ops/src/lib.rs index 01254acb4..6a16a6525 100644 --- a/crates/s3-ops/src/lib.rs +++ b/crates/s3-ops/src/lib.rs @@ -337,7 +337,7 @@ pub fn put_event_name_for_post_object(is_post_object: bool) -> EventName { /// that should omit size/etag metadata. #[inline] pub fn is_object_removed_event(event_name: EventName) -> bool { - matches!(event_name, EventName::ObjectRemovedDelete | EventName::ObjectRemovedDeleteMarkerCreated) + event_name.is_removed() } /// Returns the event mask that matches both PUT and POST object-created events. @@ -416,9 +416,21 @@ mod tests { #[test] fn test_is_object_removed_event() { + // Every ObjectRemoved* variant must be recognized as a removal event. assert!(is_object_removed_event(EventName::ObjectRemovedDelete)); assert!(is_object_removed_event(EventName::ObjectRemovedDeleteMarkerCreated)); + assert!(is_object_removed_event(EventName::ObjectRemovedDeleteAllVersions)); + assert!(is_object_removed_event(EventName::ObjectRemovedNoOP)); + assert!(is_object_removed_event(EventName::ObjectRemovedAbortMultipartUpload)); + assert!(is_object_removed_event(EventName::ObjectRemovedDeleteObjects)); + + // Non-removal events must be rejected. assert!(!is_object_removed_event(EventName::ObjectCreatedPut)); + assert!(!is_object_removed_event(EventName::ObjectCreatedPost)); + assert!(!is_object_removed_event(EventName::ObjectTaggingPut)); + assert!(!is_object_removed_event(EventName::ObjectTaggingDelete)); + assert!(!is_object_removed_event(EventName::ObjectAclPut)); + assert!(!is_object_removed_event(EventName::ObjectAccessedGet)); } #[test] diff --git a/crates/s3-types/src/event_name.rs b/crates/s3-types/src/event_name.rs index 9d213e58b..427cc9d20 100644 --- a/crates/s3-types/src/event_name.rs +++ b/crates/s3-types/src/event_name.rs @@ -185,7 +185,9 @@ impl EventName { "s3:Scanner:ManyVersions" => Ok(EventName::ScannerManyVersions), "s3:Scanner:LargeVersions" => Ok(EventName::ScannerLargeVersions), "s3:Scanner:BigPrefix" => Ok(EventName::ScannerBigPrefix), - // ObjectScannerAll and Everything cannot be parsed from strings, because the Go version also does not define their string representation. + "s3:Scanner:*" => Ok(EventName::ObjectScannerAll), + // `Everything` has no string representation (`as_str` yields ""), so it + // cannot be parsed back from a string. Every other variant round-trips. _ => Err(ParseEventNameError(s.to_string())), } } @@ -244,9 +246,8 @@ impl EventName { EventName::ScannerManyVersions => "s3:Scanner:ManyVersions", EventName::ScannerLargeVersions => "s3:Scanner:LargeVersions", EventName::ScannerBigPrefix => "s3:Scanner:BigPrefix", - // Go's String() returns "" for ObjectScannerAll and Everything - EventName::ObjectScannerAll => "s3:Scanner:*", // Follow the pattern in Go Expand - EventName::Everything => "", // Go String() returns "" to unprocessed + EventName::ObjectScannerAll => "s3:Scanner:*", // round-trips via `parse` + EventName::Everything => "", // no string form; cannot be parsed back EventName::ObjectRemovedAbortMultipartUpload => "s3:ObjectRemoved:AbortMultipartUpload", EventName::ObjectCreatedCreateMultipartUpload => "s3:ObjectCreated:CreateMultipartUpload", EventName::ObjectRemovedDeleteObjects => "s3:ObjectRemoved:DeleteObjects", @@ -334,6 +335,24 @@ impl EventName { } mask } + + /// Returns `true` for every object-removal event variant. + /// + /// Covers all `ObjectRemoved*` leaf events, including the internal + /// (metrics-only) ones, so callers can categorize removals without + /// enumerating each variant by hand. + #[inline] + pub fn is_removed(&self) -> bool { + matches!( + self, + EventName::ObjectRemovedDelete + | EventName::ObjectRemovedDeleteMarkerCreated + | EventName::ObjectRemovedDeleteAllVersions + | EventName::ObjectRemovedNoOP + | EventName::ObjectRemovedAbortMultipartUpload + | EventName::ObjectRemovedDeleteObjects + ) + } } /// Returns the S3 notification event schema version for a given event. @@ -581,6 +600,75 @@ mod tests { } } + /// Every S3-notification variant must round-trip through `as_str` -> + /// `parse`. Regression for the missing `ObjectScannerAll` parse arm + /// ("s3:Scanner:*"). + /// + /// Two groups are deliberately excluded: + /// - `Everything` has no string form (`as_str` yields ""). + /// - The internal metrics-only events are not exposed to S3 notifications + /// and intentionally have no `parse` arm. + #[test] + fn test_as_str_parse_round_trip_for_notification_variants() { + // Internal, metrics-only events: serialized but never parsed back. + let internal = [ + EventName::ObjectRemovedAbortMultipartUpload, + EventName::ObjectCreatedCreateMultipartUpload, + EventName::ObjectRemovedDeleteObjects, + ]; + + for ev in ALL_EVENT_NAMES { + if *ev == EventName::Everything { + // `Everything` intentionally serializes to "" and cannot be parsed back. + assert_eq!(ev.as_str(), ""); + assert!(EventName::parse(ev.as_str()).is_err()); + continue; + } + if internal.contains(ev) { + continue; + } + let parsed = EventName::parse(ev.as_str()); + assert_eq!(parsed.as_ref(), Ok(ev), "round-trip failed for {ev} (as_str = {:?})", ev.as_str()); + } + } + + /// `ObjectScannerAll` specifically must round-trip via "s3:Scanner:*". + #[test] + fn test_object_scanner_all_round_trips() { + assert_eq!(EventName::ObjectScannerAll.as_str(), "s3:Scanner:*"); + assert_eq!(EventName::parse("s3:Scanner:*").unwrap(), EventName::ObjectScannerAll); + } + + /// `is_removed` must be true for every `ObjectRemoved*` variant and false + /// for everything else. + #[test] + fn test_is_removed_covers_all_object_removed_variants() { + let removed = [ + EventName::ObjectRemovedDelete, + EventName::ObjectRemovedDeleteMarkerCreated, + EventName::ObjectRemovedDeleteAllVersions, + EventName::ObjectRemovedNoOP, + EventName::ObjectRemovedAbortMultipartUpload, + EventName::ObjectRemovedDeleteObjects, + ]; + for ev in removed { + assert!(ev.is_removed(), "{ev} should be classified as a removal event"); + } + + let not_removed = [ + EventName::ObjectCreatedPut, + EventName::ObjectCreatedPost, + EventName::ObjectCreatedAll, + EventName::ObjectTaggingPut, + EventName::ObjectTaggingDelete, + EventName::ObjectAclPut, + EventName::ObjectAccessedGet, + ]; + for ev in not_removed { + assert!(!ev.is_removed(), "{ev} should not be classified as a removal event"); + } + } + /// `Everything` must cover every sequential single-type bit. #[test] fn test_everything_mask_covers_all_single_types() {