Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

support checking for object modified during download #82

Merged
merged 7 commits into from
Dec 13, 2024
Merged

Conversation

TingDaoK
Copy link
Contributor

Issue #, if available:

Description of changes:

  • Adding if-match to the following downloads with the etag we received from the discovery stage.
  • If user already have the if-match in the download input, it will only succeed if it matches the etag we received. So, it's safe to overwrite it.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@TingDaoK TingDaoK marked this pull request as ready for review December 11, 2024 23:28
@TingDaoK TingDaoK requested a review from a team as a code owner December 11, 2024 23:28
Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

Approved with suggestion


/// Test the if_match header was added correctly based on the response from server.
#[tokio::test]
async fn test_download_if_match() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected behavior if the tag has changed? Can we add a test for it?

Copy link
Contributor Author

@TingDaoK TingDaoK Dec 13, 2024

Choose a reason for hiding this comment

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

I added a test for mock the object change, but the error will just be the service error from S3 that complains about if-match precondition didn't work.

In aws-c-s3, we override this error to our customized error code to inform it's because the object changed during download. But, in here, we are not overriding any service error, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't yet I think this is fine for now though unless you see an ergonomic reason to change it for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we override it for aws-c-s3 mostly because the if-match header was added by the client under the hood, so, it may be confusing to user for the if-match precondition failure.
But, not a big deal for now.

/// assertions directly on the captured requests.
/// 2. First request for discovery, succeed with etag
/// 3. Followed requests fail to mock the object changed during download.
fn mock_object_modified_connector(data: &Bytes, part_size: usize) -> StaticReplayClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this to the actual test, unless it will be used in multiple tests.

@TingDaoK TingDaoK merged commit 6e64664 into main Dec 13, 2024
13 checks passed
@TingDaoK TingDaoK deleted the if-match branch December 13, 2024 23:53
@TingDaoK TingDaoK changed the title support if-match for get objects support checking for object modified during download Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants