Skip to content

Conversation

Fred1155
Copy link
Contributor

@Fred1155 Fred1155 commented Aug 18, 2025

Motivation and Context

For the part get request for S3 transfer manager, When a response is received, the S3 Transfer Manager MUST check the value of ContentRange and validate that it matches with the expected range for the specific part number. After all requests have been sent, we should validate that the total number of part GET requests sent matches with the expected PartsCount. This PR implements the validation.

Modifications

Added validation method in MultipartDownloaderSubscriber class

Testing

Added wiremock test for failed test

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@Fred1155 Fred1155 requested a review from a team as a code owner August 18, 2025 17:42
@Fred1155 Fred1155 requested a review from zoewangg August 21, 2025 21:47
@@ -198,4 +202,14 @@ private GetObjectRequest nextRequest(int nextPartToGet) {
}
});
}

private void validatePartsCount(int currentGetCount) {
Copy link
Contributor

@zoewangg zoewangg Aug 22, 2025

Choose a reason for hiding this comment

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

Is there any way to simulate this error for testing? If not, it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test using a mock S3 client


synchronized (lock) {
if (totalParts != null && nextPartToGet > totalParts) {
log.debug(() -> String.format("Completing multipart download after a total of %d parts downloaded.", totalParts));
validatePartsCount(currentPart);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should remove validation from here because it's totally valid for publisher to send more data after subscription.

https://github.com/reactive-streams/reactive-streams-jvm

8 If a Subscription is cancelled its Subscriber MUST eventually stop being signaled.
💡 The intent of this rule is to make sure that Publishers respect a Subscriber’s request to cancel a Subscription when Subscription.cancel() has been called. The reason for eventually is because signals can have propagation delay due to being asynchronous.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably perform that check when the future is completed, and track the actual amount of time getObject was called here with a new counter instead of relying on currentPart

@zoewangg zoewangg requested review from zoewangg and removed request for zoewangg August 22, 2025 23:38
Copy link
Contributor

@L-Applin L-Applin left a comment

Choose a reason for hiding this comment

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

Can we add some tests cases for this new validation?

@Fred1155 Fred1155 requested review from millems and L-Applin August 27, 2025 18:22
@Fred1155 Fred1155 changed the title added part count and content range validation for download request added part count validation for download request Aug 28, 2025
@L-Applin
Copy link
Contributor

L-Applin commented Aug 29, 2025

I think we need a test with an actual multipart s3AsyncClient, ie:

S3AsyncClient client = S3AsyncClient client = S3AsyncClient
    .builder()
    .multipartEnabled(true)
    // other configs
    .build();

We can still mock the http client but that might be a hard test to mock, let me think about it to see if there is a way to do it.

Copy link

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.

4 participants