Skip to content

fix(services/s3): Accept List responses without ETag#3478

Merged
Xuanwo merged 3 commits intoapache:mainfrom
questdb:s3_list_without_etag
Nov 3, 2023
Merged

fix(services/s3): Accept List responses without ETag#3478
Xuanwo merged 3 commits intoapache:mainfrom
questdb:s3_list_without_etag

Conversation

@amunra
Copy link
Copy Markdown
Contributor

@amunra amunra commented Nov 3, 2023

This allows the S3 logic to accept response for path listings that don't contain the optional <ETag> xml element.

The bug was originally observed when connecting to an S3 compatible server: s3s-fs (https://crates.io/crates/s3s-fs).

I believe s3s-fs's behaviour is correct, given that the AWS S3 request/response docs state that the Etag field is optional: https://docs.aws.amazon.com/AmazonS3/latest/API/API_Object.html.

Closes #3477.

@amunra amunra requested a review from Xuanwo as a code owner November 3, 2023 16:49
@github-actions github-actions Bot added the releases-note/fix The PR fixes a bug or has a title that begins with "fix" label Nov 3, 2023
@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Nov 3, 2023

I believe s3s-fs's behaviour is correct, given that the AWS S3 request/response docs state that the Etag field is optional

I don't think this statement is correct since Key is this doc is also Required: no. And we know that it's impossible.

I'm ok with this patch to allow responses that not contain ETag. But I want to let you know that all s3 compatible services return ETag.

@amunra
Copy link
Copy Markdown
Contributor Author

amunra commented Nov 3, 2023

I believe s3s-fs's behaviour is correct, given that the AWS S3 request/response docs state that the Etag field is optional

I don't think this statement is correct since Key is this doc is also Required: no. And we know that it's impossible.

I'm ok with this patch to allow responses that not contain ETag. But I want to let you know that all s3 compatible services return ETag.

Fair :-)

For what it's worth, the official AWS Java client is flexible about this too and doesn't require the ETag either: Thank you for accepting this change!

Copy link
Copy Markdown
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.

Thanks!

@Xuanwo Xuanwo merged commit 2653963 into apache:main Nov 3, 2023
@amunra amunra deleted the s3_list_without_etag branch November 8, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Required optional ETag on .list()

2 participants