From 291404ef1962bfbcd5d8749aa1ddd6b080b8c59f Mon Sep 17 00:00:00 2001 From: Marko Grujic Date: Fri, 3 Jan 2025 15:38:02 +0100 Subject: [PATCH 1/2] Add exception to correctness checks for S3 wildcard if-none-match --- core/src/layers/correctness_check.rs | 16 ++++++++++------ core/src/services/s3/core.rs | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/core/src/layers/correctness_check.rs b/core/src/layers/correctness_check.rs index b3a08f8970fe..bc09dbf37441 100644 --- a/core/src/layers/correctness_check.rs +++ b/core/src/layers/correctness_check.rs @@ -122,12 +122,16 @@ impl LayeredAccess for CorrectnessAccessor { "if_not_exists", )); } - if args.if_none_match().is_some() && !capability.write_with_if_none_match { - return Err(new_unsupported_error( - self.info.as_ref(), - Operation::Write, - "if_none_match", - )); + if let Some(if_none_match) = args.if_none_match() { + // AWS S3 supports only wildcard (every resource) matching + let is_s3_wildcard_match = self.info.scheme() == Scheme::S3 && if_none_match == "*"; + if !is_s3_wildcard_match || !capability.write_with_if_none_match { + return Err(new_unsupported_error( + self.info.as_ref(), + Operation::Write, + "if_none_match", + )); + } } self.inner.write(path, args).await diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index f8140c5e230b..4c70757068c6 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -465,7 +465,7 @@ impl S3Core { req = req.header(IF_MATCH, if_match); } - if args.if_not_exists() { + if args.if_not_exists() || args.if_none_match() == Some("*") { req = req.header(IF_NONE_MATCH, "*"); } From 44e62ccfe6ea598c0a3ed5c79d8c5590e0fd1996 Mon Sep 17 00:00:00 2001 From: Marko Grujic Date: Fri, 3 Jan 2025 22:11:32 +0100 Subject: [PATCH 2/2] Provide an adequate error message when if-none-match-* can be replaced with if-none-exists --- core/src/layers/correctness_check.rs | 57 +++++++++++++++++++++------- core/src/services/s3/core.rs | 2 +- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/core/src/layers/correctness_check.rs b/core/src/layers/correctness_check.rs index bc09dbf37441..61d0669a393a 100644 --- a/core/src/layers/correctness_check.rs +++ b/core/src/layers/correctness_check.rs @@ -123,14 +123,14 @@ impl LayeredAccess for CorrectnessAccessor { )); } if let Some(if_none_match) = args.if_none_match() { - // AWS S3 supports only wildcard (every resource) matching - let is_s3_wildcard_match = self.info.scheme() == Scheme::S3 && if_none_match == "*"; - if !is_s3_wildcard_match || !capability.write_with_if_none_match { - return Err(new_unsupported_error( - self.info.as_ref(), - Operation::Write, - "if_none_match", - )); + if !capability.write_with_if_none_match { + let mut err = + new_unsupported_error(self.info.as_ref(), Operation::Write, "if_none_match"); + if if_none_match == "*" && capability.write_with_if_not_exists { + err = err.with_context("hint", "use if_not_exists instead"); + } + + return Err(err); } } @@ -308,7 +308,7 @@ mod tests { } async fn write(&self, _: &str, _: OpWrite) -> Result<(RpWrite, Self::Writer)> { - Ok((RpWrite::new(), Box::new(()))) + Ok((RpWrite::new(), Box::new(MockWriter))) } async fn list(&self, _: &str, _: OpList) -> Result<(RpList, Self::Lister)> { @@ -320,6 +320,22 @@ mod tests { } } + struct MockWriter; + + impl oio::Write for MockWriter { + async fn write(&mut self, _: Buffer) -> Result<()> { + Ok(()) + } + + async fn close(&mut self) -> Result<()> { + Ok(()) + } + + async fn abort(&mut self) -> Result<()> { + Ok(()) + } + } + struct MockDeleter; impl oio::Delete for MockDeleter { @@ -380,6 +396,7 @@ mod tests { async fn test_write_with() { let op = new_test_operator(Capability { write: true, + write_with_if_not_exists: true, ..Default::default() }); let res = op.write_with("path", "".as_bytes()).append(true).await; @@ -388,17 +405,31 @@ mod tests { let res = op .write_with("path", "".as_bytes()) - .if_not_exists(true) + .if_none_match("etag") .await; assert!(res.is_err()); - assert_eq!(res.unwrap_err().kind(), ErrorKind::Unsupported); + assert_eq!( + res.unwrap_err().to_string(), + "Unsupported (permanent) at write => service memory doesn't support operation write with args if_none_match" + ); + // Now try a wildcard if-none-match let res = op .write_with("path", "".as_bytes()) - .if_none_match("etag") + .if_none_match("*") .await; assert!(res.is_err()); - assert_eq!(res.unwrap_err().kind(), ErrorKind::Unsupported); + assert_eq!( + res.unwrap_err().to_string(), + "Unsupported (permanent) at write, context: { hint: use if_not_exists instead } => \ + service memory doesn't support operation write with args if_none_match" + ); + + let res = op + .write_with("path", "".as_bytes()) + .if_not_exists(true) + .await; + assert!(res.is_ok()); let op = new_test_operator(Capability { write: true, diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index 4c70757068c6..f8140c5e230b 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -465,7 +465,7 @@ impl S3Core { req = req.header(IF_MATCH, if_match); } - if args.if_not_exists() || args.if_none_match() == Some("*") { + if args.if_not_exists() { req = req.header(IF_NONE_MATCH, "*"); }