Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Return hinted error for S3 wildcard if-none-match #5506

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

gruuya
Copy link
Contributor

@gruuya gruuya commented Jan 3, 2025

Which issue does this PR close?

Closes #5505.

Rationale for this change

Work-around for S3 allowing only wildcard(*) if-none-match requests.

What changes are included in this PR?

Don't error out if the if-none-match argument is a wildcard and the scheme is S3.

Also hook-up the corresponding client argument.

Are there any user-facing changes?

if_none_match("*") and only that should work for S3.

Comment on lines 127 to 128
let is_s3_wildcard_match = self.info.scheme() == Scheme::S3 && if_none_match == "*";
if !is_s3_wildcard_match || !capability.write_with_if_none_match {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One alternative might be to generalize this to:

Suggested change
let is_s3_wildcard_match = self.info.scheme() == Scheme::S3 && if_none_match == "*";
if !is_s3_wildcard_match || !capability.write_with_if_none_match {
let if_none_match_wildcard =
if_none_match == "*" && !capability.write_with_if_not_exists;
if !if_none_match_wildcard || !capability.write_with_if_none_match {

But then this would need to be reflected in all potential clients as well.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reconsidered this and believe it’s better to return an error rather than modifying the behavior here to change the semantics of if_none_match.

Provide a helpful error message if users attempt to call if_none_match("*") on a service that supports write_with_if_not_exists but not write_with_if_none_match. In this scenario, the error should suggest that users switch to using if_not_exists instead.

@gruuya
Copy link
Contributor Author

gruuya commented Jan 3, 2025

Provide a helpful error message if users attempt to call if_none_match("*") on a service that supports write_with_if_not_exists but not write_with_if_none_match. In this scenario, the error should suggest that users switch to using if_not_exists instead.

Makes sense, I've done something along those lines, let me know what you think.

@Xuanwo Xuanwo changed the title Add exception to correctness checks for S3 wildcard if-none-match feat: Add exception to correctness checks for S3 wildcard if-none-match Jan 4, 2025
@Xuanwo Xuanwo changed the title feat: Add exception to correctness checks for S3 wildcard if-none-match feat: Return hinted error for S3 wildcard if-none-match Jan 4, 2025
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really nice, thank you @gruuya!

@Xuanwo Xuanwo merged commit d569629 into apache:main Jan 4, 2025
241 of 243 checks passed
@gruuya gruuya deleted the if-none-match-wildcard branch January 4, 2025 07:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add special checks for if_none_match("*")
2 participants