From 017317383113183ecc1de211ebcc831d6a5e9ab4 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 09:35:00 +0000 Subject: [PATCH 01/10] Allow 403 for overwrite prevention --- object_store/src/aws/client.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index 3e47abd4bcc5..bbc1716fce2f 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -485,10 +485,14 @@ impl S3Client { .send_retry(&self.config.retry_config) .await .map_err(|source| match source.status() { - Some(StatusCode::PRECONDITION_FAILED) => crate::Error::AlreadyExists { - source: Box::new(source), - path: to.to_string(), - }, + Some(StatusCode::PRECONDITION_FAILED) | Some(StatusCode::FORBIDDEN) + if !overwrite => + { + crate::Error::AlreadyExists { + source: Box::new(source), + path: to.to_string(), + } + } _ => Error::CopyRequest { source, path: from.to_string(), From 167e19a6b7b168bf3c6e2a51d427686b7cc9f64f Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 16:35:02 +0000 Subject: [PATCH 02/10] implment instead via a new 'return code override' key --- object_store/src/aws/builder.rs | 33 +++++++++++++++++++++++++++- object_store/src/aws/client.rs | 25 ++++++++++++--------- object_store/src/aws/mod.rs | 2 +- object_store/src/aws/precondition.rs | 33 +++++++++++++++++++++++++++- 4 files changed, 80 insertions(+), 13 deletions(-) diff --git a/object_store/src/aws/builder.rs b/object_store/src/aws/builder.rs index cf9490d96eae..d166b4f65a64 100644 --- a/object_store/src/aws/builder.rs +++ b/object_store/src/aws/builder.rs @@ -21,7 +21,7 @@ use crate::aws::credential::{ }; use crate::aws::{ AmazonS3, AwsCredential, AwsCredentialProvider, Checksum, S3ConditionalPut, S3CopyIfNotExists, - STORE, + S3CopyIfNotExistsReturnCodeOverride, STORE, }; use crate::client::TokenCredentialProvider; use crate::config::ConfigValue; @@ -153,6 +153,9 @@ pub struct AmazonS3Builder { skip_signature: ConfigValue, /// Copy if not exists copy_if_not_exists: Option>, + /// Copy-if-not-exists return-code override + copy_if_not_exists_return_code_override: + Option>, /// Put precondition conditional_put: Option>, /// Ignore tags @@ -293,6 +296,14 @@ pub enum AmazonS3ConfigKey { /// See [`S3CopyIfNotExists`] CopyIfNotExists, + /// Configure how to provide `copy_if_not_exists_return_code_override` + /// + /// When using [`S3CopyIfNotExists`], allows an override of the assumed return code. + /// If left unspecified, the default is `[reqwest::StatusCode::PRECONDITION_FAILED]`. + /// + /// See [`S3CopyIfNotExistsReturnCodeOverride`] + CopyIfNotExistsReturnCodeOverride, + /// Configure how to provide conditional put operations /// /// See [`S3ConditionalPut`] @@ -332,6 +343,9 @@ impl AsRef for AmazonS3ConfigKey { Self::ContainerCredentialsRelativeUri => "aws_container_credentials_relative_uri", Self::SkipSignature => "aws_skip_signature", Self::CopyIfNotExists => "aws_copy_if_not_exists", + Self::CopyIfNotExistsReturnCodeOverride => { + "aws_copy_if_not_exists_return_code_override" + } Self::ConditionalPut => "aws_conditional_put", Self::DisableTagging => "aws_disable_tagging", Self::Client(opt) => opt.as_ref(), @@ -361,6 +375,10 @@ impl FromStr for AmazonS3ConfigKey { "aws_container_credentials_relative_uri" => Ok(Self::ContainerCredentialsRelativeUri), "aws_skip_signature" | "skip_signature" => Ok(Self::SkipSignature), "aws_copy_if_not_exists" | "copy_if_not_exists" => Ok(Self::CopyIfNotExists), + "aws_copy_if_not_exists_return_code_override" + | "copy_if_not_exists_return_code_override" => { + Ok(Self::CopyIfNotExistsReturnCodeOverride) + } "aws_conditional_put" | "conditional_put" => Ok(Self::ConditionalPut), "aws_disable_tagging" | "disable_tagging" => Ok(Self::DisableTagging), // Backwards compatibility @@ -470,6 +488,10 @@ impl AmazonS3Builder { AmazonS3ConfigKey::CopyIfNotExists => { self.copy_if_not_exists = Some(ConfigValue::Deferred(value.into())) } + AmazonS3ConfigKey::CopyIfNotExistsReturnCodeOverride => { + self.copy_if_not_exists_return_code_override = + Some(ConfigValue::Deferred(value.into())) + } AmazonS3ConfigKey::ConditionalPut => { self.conditional_put = Some(ConfigValue::Deferred(value.into())) } @@ -536,6 +558,10 @@ impl AmazonS3Builder { AmazonS3ConfigKey::CopyIfNotExists => { self.copy_if_not_exists.as_ref().map(ToString::to_string) } + AmazonS3ConfigKey::CopyIfNotExistsReturnCodeOverride => self + .copy_if_not_exists_return_code_override + .as_ref() + .map(ToString::to_string), AmazonS3ConfigKey::ConditionalPut => { self.conditional_put.as_ref().map(ToString::to_string) } @@ -767,6 +793,10 @@ impl AmazonS3Builder { let region = self.region.context(MissingRegionSnafu)?; let checksum = self.checksum_algorithm.map(|x| x.get()).transpose()?; let copy_if_not_exists = self.copy_if_not_exists.map(|x| x.get()).transpose()?; + let copy_if_not_exists_return_code_override = self + .copy_if_not_exists_return_code_override + .map(|x| x.get()) + .transpose()?; let put_precondition = self.conditional_put.map(|x| x.get()).transpose()?; let credentials = if let Some(credentials) = self.credentials { @@ -875,6 +905,7 @@ impl AmazonS3Builder { disable_tagging: self.disable_tagging.get()?, checksum, copy_if_not_exists, + copy_if_not_exists_return_code_override, conditional_put: put_precondition, }; diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index bbc1716fce2f..c0a7def18ea6 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -18,7 +18,8 @@ use crate::aws::checksum::Checksum; use crate::aws::credential::{AwsCredential, CredentialExt}; use crate::aws::{ - AwsCredentialProvider, S3ConditionalPut, S3CopyIfNotExists, STORE, STRICT_PATH_ENCODE_SET, + AwsCredentialProvider, S3ConditionalPut, S3CopyIfNotExists, + S3CopyIfNotExistsReturnCodeOverride, STORE, STRICT_PATH_ENCODE_SET, }; use crate::client::get::GetClient; use crate::client::header::HeaderConfig; @@ -45,7 +46,7 @@ use percent_encoding::{utf8_percent_encode, PercentEncode}; use quick_xml::events::{self as xml_events}; use reqwest::{ header::{CONTENT_LENGTH, CONTENT_TYPE}, - Client as ReqwestClient, Method, RequestBuilder, Response, StatusCode, + Client as ReqwestClient, Method, RequestBuilder, Response, }; use serde::{Deserialize, Serialize}; use snafu::{ResultExt, Snafu}; @@ -207,6 +208,7 @@ pub struct S3Config { pub disable_tagging: bool, pub checksum: Option, pub copy_if_not_exists: Option, + pub copy_if_not_exists_return_code_override: Option, pub conditional_put: Option, } @@ -474,6 +476,13 @@ impl S3Client { } } + let acceptable_error_code = self + .config + .copy_if_not_exists_return_code_override + .clone() + .unwrap_or_default() + .0; + builder .with_aws_sigv4( credential.as_deref(), @@ -485,14 +494,10 @@ impl S3Client { .send_retry(&self.config.retry_config) .await .map_err(|source| match source.status() { - Some(StatusCode::PRECONDITION_FAILED) | Some(StatusCode::FORBIDDEN) - if !overwrite => - { - crate::Error::AlreadyExists { - source: Box::new(source), - path: to.to_string(), - } - } + Some(error) if error == acceptable_error_code => crate::Error::AlreadyExists { + source: Box::new(source), + path: to.to_string(), + }, _ => Error::CopyRequest { source, path: from.to_string(), diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index cbb3cffdf494..83c36e9f1eed 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -63,7 +63,7 @@ mod resolve; pub use builder::{AmazonS3Builder, AmazonS3ConfigKey}; pub use checksum::Checksum; -pub use precondition::{S3ConditionalPut, S3CopyIfNotExists}; +pub use precondition::{S3ConditionalPut, S3CopyIfNotExists, S3CopyIfNotExistsReturnCodeOverride}; pub use resolve::resolve_bucket_region; // http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index a50b57fe23f7..192c397246f7 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -29,7 +29,8 @@ pub enum S3CopyIfNotExists { /// /// If set, [`ObjectStore::copy_if_not_exists`] will perform a normal copy operation /// with the provided header pair, and expect the store to fail with `412 Precondition Failed` - /// if the destination file already exists + /// if the destination file already exists, or a different return code if overridden + /// using [`S3CopyIfNotExistsReturnCodeOverride`]. /// /// Encoded as `header::` ignoring whitespace /// @@ -70,6 +71,36 @@ impl Parse for S3CopyIfNotExists { } } +#[derive(Debug, Clone, Copy)] +/// Configure the return code that +pub struct S3CopyIfNotExistsReturnCodeOverride(pub reqwest::StatusCode); + +impl Default for S3CopyIfNotExistsReturnCodeOverride { + fn default() -> Self { + Self(reqwest::StatusCode::PRECONDITION_FAILED) + } +} + +impl std::fmt::Display for S3CopyIfNotExistsReturnCodeOverride { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Parse for S3CopyIfNotExistsReturnCodeOverride { + fn parse(v: &str) -> crate::Result { + ::from_str(v) + .map(S3CopyIfNotExistsReturnCodeOverride) + .map_err(|err| crate::Error::Generic { + store: "Config", + source: format!( + "Failed to parse \"{v}\" as S3CopyIfNotExistsReturnCodeOverride: {err}" + ) + .into(), + }) + } +} + /// Configure how to provide conditional put support for [`AmazonS3`]. /// /// [`AmazonS3`]: super::AmazonS3 From 8b1eb97aec2554e83e794f80b7d0a7403a1e3bc2 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 18:18:38 +0000 Subject: [PATCH 03/10] add with_... method --- object_store/src/aws/builder.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/object_store/src/aws/builder.rs b/object_store/src/aws/builder.rs index d166b4f65a64..bdf9f86eaa8d 100644 --- a/object_store/src/aws/builder.rs +++ b/object_store/src/aws/builder.rs @@ -770,6 +770,15 @@ impl AmazonS3Builder { self } + /// Configure how to provide `copy_if_not_exists` + pub fn with_copy_if_not_exists_return_code_override( + mut self, + config: S3CopyIfNotExistsReturnCodeOverride, + ) -> Self { + self.copy_if_not_exists_return_code_override = Some(config.into()); + self + } + /// Configure how to provide conditional put operations pub fn with_conditional_put(mut self, config: S3ConditionalPut) -> Self { self.conditional_put = Some(config.into()); From ee724cb9da262c71947ca2f80d8327ec1b2ef360 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 18:36:49 +0000 Subject: [PATCH 04/10] rework: implement via header-with-status --- object_store/src/aws/builder.rs | 42 +--------------------- object_store/src/aws/client.rs | 17 +++++---- object_store/src/aws/mod.rs | 2 +- object_store/src/aws/precondition.rs | 52 ++++++++++++---------------- 4 files changed, 32 insertions(+), 81 deletions(-) diff --git a/object_store/src/aws/builder.rs b/object_store/src/aws/builder.rs index bdf9f86eaa8d..cf9490d96eae 100644 --- a/object_store/src/aws/builder.rs +++ b/object_store/src/aws/builder.rs @@ -21,7 +21,7 @@ use crate::aws::credential::{ }; use crate::aws::{ AmazonS3, AwsCredential, AwsCredentialProvider, Checksum, S3ConditionalPut, S3CopyIfNotExists, - S3CopyIfNotExistsReturnCodeOverride, STORE, + STORE, }; use crate::client::TokenCredentialProvider; use crate::config::ConfigValue; @@ -153,9 +153,6 @@ pub struct AmazonS3Builder { skip_signature: ConfigValue, /// Copy if not exists copy_if_not_exists: Option>, - /// Copy-if-not-exists return-code override - copy_if_not_exists_return_code_override: - Option>, /// Put precondition conditional_put: Option>, /// Ignore tags @@ -296,14 +293,6 @@ pub enum AmazonS3ConfigKey { /// See [`S3CopyIfNotExists`] CopyIfNotExists, - /// Configure how to provide `copy_if_not_exists_return_code_override` - /// - /// When using [`S3CopyIfNotExists`], allows an override of the assumed return code. - /// If left unspecified, the default is `[reqwest::StatusCode::PRECONDITION_FAILED]`. - /// - /// See [`S3CopyIfNotExistsReturnCodeOverride`] - CopyIfNotExistsReturnCodeOverride, - /// Configure how to provide conditional put operations /// /// See [`S3ConditionalPut`] @@ -343,9 +332,6 @@ impl AsRef for AmazonS3ConfigKey { Self::ContainerCredentialsRelativeUri => "aws_container_credentials_relative_uri", Self::SkipSignature => "aws_skip_signature", Self::CopyIfNotExists => "aws_copy_if_not_exists", - Self::CopyIfNotExistsReturnCodeOverride => { - "aws_copy_if_not_exists_return_code_override" - } Self::ConditionalPut => "aws_conditional_put", Self::DisableTagging => "aws_disable_tagging", Self::Client(opt) => opt.as_ref(), @@ -375,10 +361,6 @@ impl FromStr for AmazonS3ConfigKey { "aws_container_credentials_relative_uri" => Ok(Self::ContainerCredentialsRelativeUri), "aws_skip_signature" | "skip_signature" => Ok(Self::SkipSignature), "aws_copy_if_not_exists" | "copy_if_not_exists" => Ok(Self::CopyIfNotExists), - "aws_copy_if_not_exists_return_code_override" - | "copy_if_not_exists_return_code_override" => { - Ok(Self::CopyIfNotExistsReturnCodeOverride) - } "aws_conditional_put" | "conditional_put" => Ok(Self::ConditionalPut), "aws_disable_tagging" | "disable_tagging" => Ok(Self::DisableTagging), // Backwards compatibility @@ -488,10 +470,6 @@ impl AmazonS3Builder { AmazonS3ConfigKey::CopyIfNotExists => { self.copy_if_not_exists = Some(ConfigValue::Deferred(value.into())) } - AmazonS3ConfigKey::CopyIfNotExistsReturnCodeOverride => { - self.copy_if_not_exists_return_code_override = - Some(ConfigValue::Deferred(value.into())) - } AmazonS3ConfigKey::ConditionalPut => { self.conditional_put = Some(ConfigValue::Deferred(value.into())) } @@ -558,10 +536,6 @@ impl AmazonS3Builder { AmazonS3ConfigKey::CopyIfNotExists => { self.copy_if_not_exists.as_ref().map(ToString::to_string) } - AmazonS3ConfigKey::CopyIfNotExistsReturnCodeOverride => self - .copy_if_not_exists_return_code_override - .as_ref() - .map(ToString::to_string), AmazonS3ConfigKey::ConditionalPut => { self.conditional_put.as_ref().map(ToString::to_string) } @@ -770,15 +744,6 @@ impl AmazonS3Builder { self } - /// Configure how to provide `copy_if_not_exists` - pub fn with_copy_if_not_exists_return_code_override( - mut self, - config: S3CopyIfNotExistsReturnCodeOverride, - ) -> Self { - self.copy_if_not_exists_return_code_override = Some(config.into()); - self - } - /// Configure how to provide conditional put operations pub fn with_conditional_put(mut self, config: S3ConditionalPut) -> Self { self.conditional_put = Some(config.into()); @@ -802,10 +767,6 @@ impl AmazonS3Builder { let region = self.region.context(MissingRegionSnafu)?; let checksum = self.checksum_algorithm.map(|x| x.get()).transpose()?; let copy_if_not_exists = self.copy_if_not_exists.map(|x| x.get()).transpose()?; - let copy_if_not_exists_return_code_override = self - .copy_if_not_exists_return_code_override - .map(|x| x.get()) - .transpose()?; let put_precondition = self.conditional_put.map(|x| x.get()).transpose()?; let credentials = if let Some(credentials) = self.credentials { @@ -914,7 +875,6 @@ impl AmazonS3Builder { disable_tagging: self.disable_tagging.get()?, checksum, copy_if_not_exists, - copy_if_not_exists_return_code_override, conditional_put: put_precondition, }; diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index c0a7def18ea6..bb6c6dc414ab 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -18,8 +18,7 @@ use crate::aws::checksum::Checksum; use crate::aws::credential::{AwsCredential, CredentialExt}; use crate::aws::{ - AwsCredentialProvider, S3ConditionalPut, S3CopyIfNotExists, - S3CopyIfNotExistsReturnCodeOverride, STORE, STRICT_PATH_ENCODE_SET, + AwsCredentialProvider, S3ConditionalPut, S3CopyIfNotExists, STORE, STRICT_PATH_ENCODE_SET, }; use crate::client::get::GetClient; use crate::client::header::HeaderConfig; @@ -208,7 +207,6 @@ pub struct S3Config { pub disable_tagging: bool, pub checksum: Option, pub copy_if_not_exists: Option, - pub copy_if_not_exists_return_code_override: Option, pub conditional_put: Option, } @@ -468,6 +466,9 @@ impl S3Client { Some(S3CopyIfNotExists::Header(k, v)) => { builder = builder.header(k, v); } + Some(S3CopyIfNotExists::HeaderWithStatus(k, v, _)) => { + builder = builder.header(k, v); + } None => { return Err(crate::Error::NotSupported { source: "S3 does not support copy-if-not-exists".to_string().into(), @@ -476,12 +477,10 @@ impl S3Client { } } - let acceptable_error_code = self - .config - .copy_if_not_exists_return_code_override - .clone() - .unwrap_or_default() - .0; + let acceptable_error_code = match &self.config.copy_if_not_exists { + Some(S3CopyIfNotExists::HeaderWithStatus(_, _, code)) => code.clone(), + _ => reqwest::StatusCode::PRECONDITION_FAILED, + }; builder .with_aws_sigv4( diff --git a/object_store/src/aws/mod.rs b/object_store/src/aws/mod.rs index 83c36e9f1eed..cbb3cffdf494 100644 --- a/object_store/src/aws/mod.rs +++ b/object_store/src/aws/mod.rs @@ -63,7 +63,7 @@ mod resolve; pub use builder::{AmazonS3Builder, AmazonS3ConfigKey}; pub use checksum::Checksum; -pub use precondition::{S3ConditionalPut, S3CopyIfNotExists, S3CopyIfNotExistsReturnCodeOverride}; +pub use precondition::{S3ConditionalPut, S3CopyIfNotExists}; pub use resolve::resolve_bucket_region; // http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index 192c397246f7..11ac44135694 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -39,12 +39,18 @@ pub enum S3CopyIfNotExists { /// /// [`ObjectStore::copy_if_not_exists`]: crate::ObjectStore::copy_if_not_exists Header(String, String), + /// The same as `Header` but allows custom status code checking, for object stores that return values + /// other than 412. + HeaderWithStatus(String, String, reqwest::StatusCode), } impl std::fmt::Display for S3CopyIfNotExists { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Header(k, v) => write!(f, "header: {}: {}", k, v), + Self::HeaderWithStatus(k, v, code) => { + write!(f, "header-with-status: {}: {k}: {v}", code.as_u16()) + } } } } @@ -57,6 +63,22 @@ impl S3CopyIfNotExists { let (k, v) = value.split_once(':')?; Some(Self::Header(k.trim().to_string(), v.trim().to_string())) } + "header-with-status" => { + let parts = value.split(':').collect::>(); + + if parts.len() != 3 { + // format should be `header-with_status: 999: key: value` + return None; + } + + let code = ::from_str(&parts[0]).ok()?; + + Some(Self::HeaderWithStatus( + parts[1].trim().to_string(), + parts[2].trim().to_string(), + code, + )) + } _ => None, } } @@ -71,36 +93,6 @@ impl Parse for S3CopyIfNotExists { } } -#[derive(Debug, Clone, Copy)] -/// Configure the return code that -pub struct S3CopyIfNotExistsReturnCodeOverride(pub reqwest::StatusCode); - -impl Default for S3CopyIfNotExistsReturnCodeOverride { - fn default() -> Self { - Self(reqwest::StatusCode::PRECONDITION_FAILED) - } -} - -impl std::fmt::Display for S3CopyIfNotExistsReturnCodeOverride { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl Parse for S3CopyIfNotExistsReturnCodeOverride { - fn parse(v: &str) -> crate::Result { - ::from_str(v) - .map(S3CopyIfNotExistsReturnCodeOverride) - .map_err(|err| crate::Error::Generic { - store: "Config", - source: format!( - "Failed to parse \"{v}\" as S3CopyIfNotExistsReturnCodeOverride: {err}" - ) - .into(), - }) - } -} - /// Configure how to provide conditional put support for [`AmazonS3`]. /// /// [`AmazonS3`]: super::AmazonS3 From 0e9cff4a4354ac526131f1f630d376b0de828098 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 19:21:35 +0000 Subject: [PATCH 05/10] Update object_store/src/aws/precondition.rs Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> --- object_store/src/aws/precondition.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index 11ac44135694..12f4486f850a 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -71,7 +71,7 @@ impl S3CopyIfNotExists { return None; } - let code = ::from_str(&parts[0]).ok()?; + let code = parts[0].trim().parse().ok()?; Some(Self::HeaderWithStatus( parts[1].trim().to_string(), From ef43c4aaae7768231def9aa98a3ef51aa4493975 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 19:22:43 +0000 Subject: [PATCH 06/10] Update object_store/src/aws/precondition.rs Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> --- object_store/src/aws/precondition.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index 12f4486f850a..a805384ca330 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -41,6 +41,8 @@ pub enum S3CopyIfNotExists { Header(String, String), /// The same as `Header` but allows custom status code checking, for object stores that return values /// other than 412. + /// + /// Encoded as `header-with-status:::` ignoring whitespace HeaderWithStatus(String, String, reqwest::StatusCode), } From 5e905978f7b17ec7ff34218b3d6a776e4f469d43 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 19:22:52 +0000 Subject: [PATCH 07/10] Update object_store/src/aws/precondition.rs Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> --- object_store/src/aws/precondition.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index a805384ca330..f1d71cab2686 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -66,7 +66,7 @@ impl S3CopyIfNotExists { Some(Self::Header(k.trim().to_string(), v.trim().to_string())) } "header-with-status" => { - let parts = value.split(':').collect::>(); + let (k, v, status) = value.split(':').collect_tuple()?; if parts.len() != 3 { // format should be `header-with_status: 999: key: value` From c4bfde662580585619c7787307cb383f916b414c Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 19:23:02 +0000 Subject: [PATCH 08/10] Update object_store/src/aws/client.rs Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com> --- object_store/src/aws/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index bb6c6dc414ab..73e5116545a2 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -477,7 +477,7 @@ impl S3Client { } } - let acceptable_error_code = match &self.config.copy_if_not_exists { + let precondition_failure = match &self.config.copy_if_not_exists { Some(S3CopyIfNotExists::HeaderWithStatus(_, _, code)) => code.clone(), _ => reqwest::StatusCode::PRECONDITION_FAILED, }; From 3ee6fb6b4f75852def464fc9a209a4e347b6be07 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 21:44:46 +0000 Subject: [PATCH 09/10] review comments --- object_store/src/aws/client.rs | 2 +- object_store/src/aws/precondition.rs | 90 ++++++++++++++++++++++++---- 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index 73e5116545a2..56c06f16c0eb 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -493,7 +493,7 @@ impl S3Client { .send_retry(&self.config.retry_config) .await .map_err(|source| match source.status() { - Some(error) if error == acceptable_error_code => crate::Error::AlreadyExists { + Some(error) if error == precondition_failure => crate::Error::AlreadyExists { source: Box::new(source), path: to.to_string(), }, diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index f1d71cab2686..7e905dc63f70 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -17,11 +17,13 @@ use crate::config::Parse; +use itertools::Itertools; + /// Configure how to provide [`ObjectStore::copy_if_not_exists`] for [`AmazonS3`]. /// /// [`ObjectStore::copy_if_not_exists`]: crate::ObjectStore::copy_if_not_exists /// [`AmazonS3`]: super::AmazonS3 -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum S3CopyIfNotExists { /// Some S3-compatible stores, such as Cloudflare R2, support copy if not exists @@ -51,7 +53,7 @@ impl std::fmt::Display for S3CopyIfNotExists { match self { Self::Header(k, v) => write!(f, "header: {}: {}", k, v), Self::HeaderWithStatus(k, v, code) => { - write!(f, "header-with-status: {}: {k}: {v}", code.as_u16()) + write!(f, "header-with-status: {k}: {v}: {}", code.as_u16()) } } } @@ -68,16 +70,11 @@ impl S3CopyIfNotExists { "header-with-status" => { let (k, v, status) = value.split(':').collect_tuple()?; - if parts.len() != 3 { - // format should be `header-with_status: 999: key: value` - return None; - } - - let code = parts[0].trim().parse().ok()?; + let code = status.trim().parse().ok()?; Some(Self::HeaderWithStatus( - parts[1].trim().to_string(), - parts[2].trim().to_string(), + k.trim().to_string(), + v.trim().to_string(), code, )) } @@ -136,3 +133,76 @@ impl Parse for S3ConditionalPut { }) } } + +#[cfg(test)] +mod tests { + use super::S3CopyIfNotExists; + + #[test] + fn parse_s3_copy_if_not_exists_header() { + let input = "header: cf-copy-destination-if-none-match: *"; + let expected = Some(S3CopyIfNotExists::Header( + "cf-copy-destination-if-none-match".to_owned(), + "*".to_owned(), + )); + + assert_eq!(expected, S3CopyIfNotExists::from_str(input)); + } + + #[test] + fn parse_s3_copy_if_not_exists_header_with_status() { + let input = "header-with-status:key:value:403"; + let expected = Some(S3CopyIfNotExists::HeaderWithStatus( + "key".to_owned(), + "value".to_owned(), + reqwest::StatusCode::FORBIDDEN, + )); + + assert_eq!(expected, S3CopyIfNotExists::from_str(input)); + } + + #[test] + fn parse_s3_copy_if_not_exists_header_whitespace_invariant() { + let expected = Some(S3CopyIfNotExists::Header( + "cf-copy-destination-if-none-match".to_owned(), + "*".to_owned(), + )); + + const INPUTS: &[&str] = &[ + "header:cf-copy-destination-if-none-match:*", + "header: cf-copy-destination-if-none-match:*", + "header: cf-copy-destination-if-none-match: *", + "header : cf-copy-destination-if-none-match: *", + "header : cf-copy-destination-if-none-match : *", + "header : cf-copy-destination-if-none-match : * ", + ]; + + for input in INPUTS { + assert_eq!(expected, S3CopyIfNotExists::from_str(input)); + } + } + + #[test] + fn parse_s3_copy_if_not_exists_header_with_status_whitespace_invariant() { + let expected = Some(S3CopyIfNotExists::HeaderWithStatus( + "key".to_owned(), + "value".to_owned(), + reqwest::StatusCode::FORBIDDEN, + )); + + const INPUTS: &[&str] = &[ + "header-with-status:key:value:403", + "header-with-status: key:value:403", + "header-with-status: key: value:403", + "header-with-status: key: value: 403", + "header-with-status : key: value: 403", + "header-with-status : key : value: 403", + "header-with-status : key : value : 403", + "header-with-status : key : value : 403 ", + ]; + + for input in INPUTS { + assert_eq!(expected, S3CopyIfNotExists::from_str(input)); + } + } +} From 3f7407b28fb1371f3c59aab53f85fdaf9b053f21 Mon Sep 17 00:00:00 2001 From: emcake <3726783+emcake@users.noreply.github.com> Date: Tue, 28 Nov 2023 22:34:47 +0000 Subject: [PATCH 10/10] clipps lints & docs --- object_store/src/aws/client.rs | 2 +- object_store/src/aws/precondition.rs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/object_store/src/aws/client.rs b/object_store/src/aws/client.rs index 56c06f16c0eb..ecbe556c6dfe 100644 --- a/object_store/src/aws/client.rs +++ b/object_store/src/aws/client.rs @@ -478,7 +478,7 @@ impl S3Client { } let precondition_failure = match &self.config.copy_if_not_exists { - Some(S3CopyIfNotExists::HeaderWithStatus(_, _, code)) => code.clone(), + Some(S3CopyIfNotExists::HeaderWithStatus(_, _, code)) => *code, _ => reqwest::StatusCode::PRECONDITION_FAILED, }; diff --git a/object_store/src/aws/precondition.rs b/object_store/src/aws/precondition.rs index 7e905dc63f70..ada5f3b83f07 100644 --- a/object_store/src/aws/precondition.rs +++ b/object_store/src/aws/precondition.rs @@ -31,8 +31,7 @@ pub enum S3CopyIfNotExists { /// /// If set, [`ObjectStore::copy_if_not_exists`] will perform a normal copy operation /// with the provided header pair, and expect the store to fail with `412 Precondition Failed` - /// if the destination file already exists, or a different return code if overridden - /// using [`S3CopyIfNotExistsReturnCodeOverride`]. + /// if the destination file already exists. /// /// Encoded as `header::` ignoring whitespace /// @@ -41,7 +40,7 @@ pub enum S3CopyIfNotExists { /// /// [`ObjectStore::copy_if_not_exists`]: crate::ObjectStore::copy_if_not_exists Header(String, String), - /// The same as `Header` but allows custom status code checking, for object stores that return values + /// The same as [`S3CopyIfNotExists::Header`] but allows custom status code checking, for object stores that return values /// other than 412. /// /// Encoded as `header-with-status:::` ignoring whitespace