fix: support AWS_REQUEST_PAYER=requester

[ljstrnadiii]

Which issue does this PR close?

Closes #668

Rationale for this change

Avoid AWS_REQUEST_PAYER env var collision.

What changes are included in this PR?

Resolves 'requester' to True for requester_pays

Are there any user-facing changes?

No.

[crepererum]

the parsing logic should be part of the value type, like this:

arrow-rs-object-store/src/aws/builder.rs

Lines 1330 to 1365 in bdcac43

	#[derive(Debug, Clone)]
	enum S3EncryptionType {
	S3,
	SseKms,
	DsseKms,
	SseC,
	}
	impl crate::config::Parse for S3EncryptionType {
	fn parse(s: &str) -> Result<Self> {
	match s {
	"AES256" => Ok(Self::S3),
	"aws:kms" => Ok(Self::SseKms),
	"aws:kms:dsse" => Ok(Self::DsseKms),
	"sse-c" => Ok(Self::SseC),
	_ => Err(Error::InvalidEncryptionType { passed: s.into() }.into()),
	}
	}
	}
	impl From<&S3EncryptionType> for &'static str {
	fn from(value: &S3EncryptionType) -> Self {
	match value {
	S3EncryptionType::S3 => "AES256",
	S3EncryptionType::SseKms => "aws:kms",
	S3EncryptionType::DsseKms => "aws:kms:dsse",
	S3EncryptionType::SseC => "sse-c",
	}
	}
	}
	impl std::fmt::Display for S3EncryptionType {
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	f.write_str(self.into())
	}
	}

Now from the user PoV this doesn't make a difference, but using this pattern makes the code easier to read and maintain (no surprising "why is this value handled differently and where is the code that does this out of the sudden?" surprises)

[ljstrnadiii]

Thanks for the guidance. Will make this change today.

[kylebarron]

@crepererum Putting this as part of the impl Parse for bool

arrow-rs-object-store/src/config.rs

Lines 74 to 86 in bdcac43

	impl Parse for bool {
	fn parse(v: &str) -> Result<Self> {
	let lower = v.to_ascii_lowercase();
	match lower.as_str() {
	"1" | "true" | "on" | "yes" | "y" => Ok(true),
	"0" | "false" | "off" | "no" | "n" => Ok(false),
	_ => Err(Error::Generic {
	store: "Config",
	source: format!("failed to parse \"{v}\" as boolean").into(),
	}),
	}
	}
	}

means that any boolean flag is enabled by the string "requester", right?

From my point of view if we allow this case, it seems perhaps better to isolate it to the AWS_REQUEST_PAYER code path? 🤷‍♂️

[ljstrnadiii]

I also went back and forth on this, but @kylebarron has a good point. I'll wait for followup from @crepererum before I do anything here.

[crepererum]

I would probably use a new-type wrapper for that, like:

// derive whatever you need, but basically this is just a `bool`
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash]
struct RequesterPayer(bool);

impl Parse for RequesterPayer {
    fn parse(v: &str) -> Result<Self> {
        if v.eq_ignore_ascii_case("requester") {
            Ok(Self(true))
        } else {
            Ok(Self(bool::parse(v)?))
        }
    }
}

// you may want to implement a few convenience traits
impl From<RequesterPayer> for bool {...}

impl AsRef<bool> for RequesterPayer {...}

impl Deref for RequesterPayer {...}

[kylebarron]

The newtype wrapper sounds great.

[ljstrnadiii]

Roger that. Thanks all.

[crepererum]

Small nitpicks, but overall this looks pretty good already.

[crepererum]

Can I get a case-insensitive test case too, e.g. Requester or REQUESTER?

[crepererum]

Suggested change

	#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
	/// Captures `AWS_REQUEST_PAYER`.
	///
	/// Parses either as `"requester"` (case-insensitive) meaning `true`, or as a [`bool`] (i.e. `"true"`, `"1"`, `"no"`, etc.).
	#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]

Feel free to rephrase, but I think leaving some documentation might be nice.

[ljstrnadiii]

@crepererum can we merge?

[crepererum]

Are you OK if we put this into the next major release (0.14.x) or do you need this as part of the 0.13.x series?

[kylebarron]

I think @ljstrnadiii might just need this exposed through Obstore, which we can do by publishing a Python version on this Git tag. Or do you need this as a rust release @ljstrnadiii?

[ljstrnadiii]

Correct, we need this through obstore.

[kylebarron]

Good to know. Feel free to make an issue in obstore to update to this Git hash. Ideally we'd go commit-by-commit so that we ensure we expose all underlying features through to obstore