feat: add upload functionality #214
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #214 +/- ##
============================================
+ Coverage 63.50% 63.67% +0.17%
- Complexity 540 548 +8
============================================
Files 30 31 +1
Lines 4759 4782 +23
Branches 427 428 +1
============================================
+ Hits 3022 3045 +23
Misses 1577 1577
Partials 160 160
Continue to review full report at Codecov.
|
| * } | ||
| * }</pre> | ||
| */ | ||
| public final class StorageUtils { |
There was a problem hiding this comment.
"Utils" classes usually don't have object state or non-static methods. Perhaps there's a better name here? Uploader or StorageUploader perhaps?
There was a problem hiding this comment.
This class could be used not only of uploading, it could contain downloading or another functionality as well. Would it be okay to rename it to StorageHelper?
There was a problem hiding this comment.
That works. Another possibility: StorageTransmitter.
However, also consider whether the Storage field works better as a method argument.
There was a problem hiding this comment.
I think it's a very rare case when someone has more than one instance of the storage. So, passing this instance to every method doesn't seem to be convenient. Methods with 4 arguments doesn't look as easy to understand.
Having a class above the storage will certainly give some freedom in extending API without breaking compatibility. To me 'Transmitter' sounds like something closed to the physical layer. 'Helper' is very generic. May be StorageOperations or StorageFunctions fits better?
There was a problem hiding this comment.
StorageMover? StorageTransporter?
There was a problem hiding this comment.
I like StorageOperations, actually.
| /** | ||
| * Utility methods to perform various operations with the Storage such as upload. | ||
| * | ||
| * <p>Example of uploading files from a folder: |
There was a problem hiding this comment.
"directory" might or might not be clearer here. Depending on whether the reader is coming from Mac, Windows, or Linux. However Java does call this directory, so we should probably stick to that.
| * StorageUtils utils = StorageUtils.create(storage); | ||
| * for (File file: folder.listFiles()) { | ||
| * if (!file.isDirectory()) { | ||
| * BlobInfo blobInfo = BlobInfo.newBuilder(BUCKET, file.getName()).build(); |
There was a problem hiding this comment.
two space indents in example
| * } | ||
| * }</pre> | ||
| */ | ||
| public final class StorageUtils { |
There was a problem hiding this comment.
StorageMover? StorageTransporter?
| public final class StorageUtils { | ||
|
|
||
| /** The instance of the Storage the utilities are associated with. */ | ||
| public final Storage storage; |
There was a problem hiding this comment.
StorageMover? StorageTransporter?
This class can be used not only for upload/download operation, but for any operation with the storage. That will allow to add new functionality without extending the Storage interface and breaking API.
May be StorageOperations or StorageFunctions feet better?
| * @param storage the Storage | ||
| * @return an instance which refers to {@code storage} | ||
| */ | ||
| public static StorageUtils create(Storage storage) { |
There was a problem hiding this comment.
This is no better than a constructor. Remove it.
|
|
||
| /** | ||
| * Uploads the given {@code path} to the blob using {@link Storage#writer} and the given {@code | ||
| * bufferSize}. By default any MD5 and CRC32C values in the given {@code blobInfo} are ignored |
| * unless requested via the {@link Storage.BlobWriteOption#md5Match()} and {@link | ||
| * Storage.BlobWriteOption#crc32cMatch()} options. Folder upload is not supported. | ||
| * | ||
| * <p>{@link #upload(BlobInfo, Path, Storage.BlobWriteOption...)} invokes this one with a buffer |
| } | ||
|
|
||
| /** | ||
| * Uploads the given {@code content} to the blob using {@link Storage#writer}. By default any MD5 |
There was a problem hiding this comment.
"Uploads the given {@code content}" --> Reads bytes from an input stream and uploads those bytes to the blob
| * | ||
| * <pre>{@code | ||
| * BlobId blobId = BlobId.of(bucketName, blobName); | ||
| * byte[] content = "Hello, world".getBytes(UTF_8); |
| * <pre>{@code | ||
| * File folder = new File("pictures/"); | ||
| * StorageUtils utils = StorageUtils.create(storage); | ||
| * for (File file: folder.listFiles()) { |
There was a problem hiding this comment.
I'm not sure why you have this example at all. It doesn't handle subdirectories.
There was a problem hiding this comment.
This example demonstrates that:
- StorageUtils instance can be reused
- Upload of folders is not supported
To upload a folder with subfolders one should case about nuances like correct handling of symbolic links. Getting the blob name for files in subfolders requires an extra code. So the example will be too complicated.
I can remove this one.
|
@elharo thanks a lot for your comments |
Fixes #179