fix(s3 dataplane): Fix transfer of empty objects #417
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
============================================
+ Coverage 63.82% 65.53% +1.70%
- Complexity 0 117 +117
============================================
Files 26 28 +2
Lines 633 676 +43
Branches 30 32 +2
============================================
+ Hits 404 443 +39
+ Misses 222 218 -4
- Partials 7 15 +8 ☔ View full report in Codecov by Sentry. |
|
While discussing the changes with @bmg13 we identified another case where having nested folder files would brake the tests. We accounted for that case in the last commit. @paullatzelsperger This is ready for review. I can't add properly request the review... |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...data-plane-aws-s3/src/main/java/org/eclipse/edc/connector/dataplane/aws/s3/S3DataSource.java
Fixed
Show fixed
Hide fixed
| @@ -116,8 +118,12 @@ private List<S3Object> fetchPrefixedS3Objects() { | |||
| return s3Objects; | |||
| } | |||
|
|
|||
| private Collection<S3Object> filterOutFolderFile(List<S3Object> contents) { | |||
| return contents.stream().filter(object -> !object.key().endsWith("/")).collect(Collectors.toSet()); | |||
There was a problem hiding this comment.
why does it transform a List to a Set?
There was a problem hiding this comment.
Initial idea was to avoid duplicates. Thinking about it a second time and a files full name cannot be a duplicate, so this transformation makes no sense.
There was a problem hiding this comment.
changed in 62ef21a
| @@ -116,8 +118,12 @@ private List<S3Object> fetchPrefixedS3Objects() { | |||
| return s3Objects; | |||
| } | |||
|
|
|||
| private Collection<S3Object> filterOutFolderFile(List<S3Object> contents) { | |||
| return contents.stream().filter(object -> !object.key().endsWith("/")).collect(Collectors.toSet()); | |||
There was a problem hiding this comment.
the filter could be extracted in a field/constant like:
Predicate<S3Object> isFile = object -> !object.key.endsWith("/");
and this method could be inlined
There was a problem hiding this comment.
changed in 62ef21a
| @@ -105,6 +107,12 @@ void should_copy_using_destination_object_name_case_single_transfer(List<String> | |||
| var objectNameInDestination = "object-name-in-destination"; | |||
| var objectContent = UUID.randomUUID().toString(); | |||
|
|
|||
| //Put folder 0 byte size file marker. AWS does this when a folder is created via the console. | |||
| if (!isSingleObject) { | |||
There was a problem hiding this comment.
I know the test was already structured like this, but it's not a good practice to have conditionals into a test, better to have 2 distinct tests. It could also be solved in a different PR
There was a problem hiding this comment.
I agree these test class deserve some attention. The conditionals are there because the argument provider is the same for both test cases. I maintained it the same way to avoid refactoring the thing since it seems to be better done in another PR. I can create the issue.
| @@ -107,7 +109,9 @@ private List<S3Object> fetchPrefixedS3Objects() { | |||
|
|
|||
| var response = client.listObjectsV2(listObjectsRequest); | |||
|
|
|||
| s3Objects.addAll(response.contents()); | |||
| Predicate<S3Object> isFile = object -> !object.key().endsWith("/"); | |||
There was a problem hiding this comment.
this can be a class field or constant, so it won't be recreated each time
There was a problem hiding this comment.
addressed in a6645ac
What this PR changes/adds
When creating a folder via the AWS console, a 0 byte object with the name of the folder is created.
This object is picked up by the s3 listObjects call and passed to
transferPartsas a valid part to be transfered. This creates an unnecessary Part to be uploaded which can be filtered out.Also, the multipart upload logic wasn't able to upload 0 byte files since no empty
completedPartslist can be used in acompletedMultipartUploadrequest.Why it does that
To enable the transfer of 0 byte files and remove the unecessary upload of folder 0 byte marker.
Further notes
Some tests where adapted to best represent the testing cases.
Linked Issue(s)
Closes #384