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

1005 S3 - Tagging S3 Objects using the TransferManager uploadDirectory #1011

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@erbrito

erbrito commented Feb 2, 2017

#1005 adding the ObjectTaggingProvider Interface to be used to specify the tagging (the same way it is used to specify the callback to specify the metadata to each file).
Adding 2 methods to the TransferManager to upload a directory and a list of files using the ObjectTaggingProvider

@dagnir

Let's just delegate to the new overload here and just pass in a null for objectTaggingProvider

* The tags for the file. You can modify this object to specify
* your own tags.
*/
public void provideObjectTags(final File file, final ObjectTagging tags);

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

Let's go ahead and change the signature to public TagSet provideObjectTags(File file);

This comment has been minimized.

@erbrito

erbrito Feb 2, 2017

I would like to keep it consistent with the ObjectMetadataProvider signature: public void provideObjectMetadata(final File file, final ObjectMetadata metadata);https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/transfer/ObjectMetadataProvider.java

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

Any particular reason why? Using out parameters is not a very good pattern (but we can't change ObjectMetatadaProvider now) and can be cleanly avoided here. Using a lambda here on Java 8+ would also be more natural.

This comment has been minimized.

@erbrito

erbrito Feb 2, 2017

only to keep it consistent, as will be more natural to implement those interfaces in the same way. I'll do the change based on your feedback.

This comment has been minimized.

@erbrito

erbrito Feb 2, 2017

Changes are done; however, I'm returning ObjectTagging as TagSet is a map and ObjectTagging is creaged from a list of Tags. And is the one that is used on the TransferManager to make the request. If you prefer, I can return a List<tags> instead. Please advise

@@ -1335,6 +1337,40 @@ public MultipleFileUpload uploadDirectory(String bucketName, String virtualDirec
return uploadFileList(bucketName, virtualDirectoryKeyPrefix, directory, files, metadataProvider);
}
/**
*

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

Please add a description

* files found in subdirectories will be included with an
* appropriate concatenation to the key prefix.
* @param metadataProvider
* A callback of type <code>ObjectMetadataProvider</code> which

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

nit: indenting is a little weird here

* @param taggingProvider
* @return
*/
public MultipleFileUpload uploadFileList(String bucketName, String virtualDirectoryKeyPrefix, File directory, List<File> files, ObjectMetadataProvider metadataProvider, ObjectTaggingProvider taggingProvider) {

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

Let's change the original uploadFileList to just delegate to this one

This comment has been minimized.

@erbrito

erbrito Feb 3, 2017

it is done at line 1449

@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 2, 2017

Thanks for the Pull Request! Overall it looks good, just a few tweaks. Also can you please confirm that you're submitting these changes under the Apache 2.0 License?

import java.io.File;
/**
* This is the callback interface which is used by TransferManager.uploadDirectory and

This comment has been minimized.

@dagnir

dagnir Feb 2, 2017

Contributor

minor: Let's change to {@link TransferManager#uploadDirectory} so a link is created in the javadocs

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Please make this change as well

Erick Brito
1) change original uploadFileList and uploadDirectory to delegate to …
…the new ones.

2) fix comments identations and missing description.
Based on pull request review
@erbrito

This comment has been minimized.

erbrito commented Feb 2, 2017

@dagnir I confirm, all code is under the same licence AWS-SDK-JAVA is (Apache 2). Should I add the license?`/*

  • Copyright 2010-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
  • Licensed under the Apache License, Version 2.0 (the "License").
  • You may not use this file except in compliance with the License.
  • A copy of the License is located at
  • http://aws.amazon.com/apache2.0
  • or in the "license" file accompanying this file. This file is distributed
  • on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
  • express or implied. See the License for the specific language governing
  • permissions and limitations under the License.
    */`

@erbrito erbrito closed this Feb 2, 2017

Erick Brito
change signature to method on ObjectTaggingProvider to return the
ObjectTagging. change on TransferManager to use new signature. (Based on
comments on pull request).
@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 2, 2017

Yep, please add the header. Thank you!

@dagnir dagnir reopened this Feb 2, 2017

@erbrito

This comment has been minimized.

erbrito commented Feb 2, 2017

@dagnir I committed the changes based on your feedback. Thanks for it. Let me know if something is missing.

@@ -1458,13 +1526,20 @@ public MultipleFileUpload uploadFileList(String bucketName, String virtualDirect
.replaceAll("\\\\", "/");
ObjectMetadata metadata = new ObjectMetadata();
ObjectTagging objectTagging = new ObjectTagging(new ArrayList<Tag>());

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Initialize to null

import java.io.File;
/**
* This is the callback interface which is used by TransferManager.uploadDirectory and

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Please make this change as well

Erick Brito added some commits Feb 3, 2017

Erick Brito
Initializing ObjectTagging to null at TransferManager.uploadFileList
when preparing the PutObjectRequest before the call to the
taggingProvider.
import java.util.List;
/**
* This is the callback interface which is used by TransferManager.uploadDirectory {@link TransferManager#uploadDirectory(String, String, File, boolean, ObjectMetadataProvider, ObjectTaggingProvider)} and

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Some minor things:

  • Remove "TransferManager.uploadDirectory" and "TransferManager.uploadFileList" and just leave the {@link}s
  • The @link for uploadDirectory is the wrong overload, it should be the new one with the ObjectTaggingProvider param
  • wrap the javadoc 100 characters per line

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Oops disregard the second bullet point, it's actually the right one

}
/**
*

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Javadoc description

@@ -41,6 +41,8 @@
import com.amazonaws.services.s3.model.ObjectMetadata;
import com.amazonaws.services.s3.model.PutObjectRequest;
import com.amazonaws.services.s3.model.S3ObjectSummary;
import com.amazonaws.services.s3.model.ObjectTagging;
import com.amazonaws.services.s3.model.Tag;

This comment has been minimized.

@dagnir

dagnir Feb 3, 2017

Contributor

Remove this unused import

@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 3, 2017

Thanks for the changes! This is nearly there, just had a few more minor things.

@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 4, 2017

Looks good. I've pulled the code in internally and sent it for review with the rest of the team.

@erbrito

This comment has been minimized.

erbrito commented Feb 6, 2017

@dagnir If all is good, when do you think it will be merged and available (released). I would like to use this feature throw the AWS-JAVA-SDK. Thanks.

@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 6, 2017

Hey @erbrito, the changes are still pending review internally, but it should be merged by the end of the week.

@erbrito

This comment has been minimized.

erbrito commented Feb 6, 2017

@dagnir Thanks for the feedback.

@dagnir

This comment has been minimized.

Contributor

dagnir commented Feb 10, 2017

Hi @erbrito I opened #1020 after we reviewed internally to make some minor modifications on top of your changes. Closing this one.

@dagnir dagnir closed this Feb 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment