Merge Storage Blob Client Builders #4468
Conversation
alzimmermsft
added
Client
alzimmermsft
requested review from
JonathanGiles,
hemanttanwar,
jaschrep-msft,
jianghaolu and
rickle-msft
jaschrep-msft
left a comment
jaschrep-msft
left a comment
There was a problem hiding this comment.
Minor docstring additions needed. Discussion on behavior of BlobClientBuilder::endpoint() is necessary. Results may need to be applied to ContainerClientBuilder.
| */ | ||
| public BlobClient buildClient() { | ||
| return new BlobClient(buildAsyncClient()); | ||
| public BlobClient buildBlobClient() { |
There was a problem hiding this comment.
Now that we build all four types of blob clients from the same builder, we should have the docstrings each have a little something of a main description talking about what the clients are for. Specifically, I want them to touch on how Blob(Async)Client is for generic blob methods like download and properties for when you don't know what kind of blob it is, and that the other three are for use when you know the type of blob you are interacting with.
There was a problem hiding this comment.
Added comments to explain when to build which client.
| * @throws IllegalArgumentException If {@code endpoint} is a malformed URL. | ||
| * @throws IllegalArgumentException If {@code endpoint} is {@code null} or is a malformed URL. | ||
| */ | ||
| public BlobClientBuilder endpoint(String endpoint) { |
There was a problem hiding this comment.
This method needs to be able to accept a fully formed URL to a blob resource. This only takes in the scheme, host, snapshot, and SAS; the path is never looked at.
There was a problem hiding this comment.
Good catch, I'll clean that up.
There was a problem hiding this comment.
Turns out the path is handled by the URLParser, it is turned into containerName and blobName pieces.
|
|
||
| if (parts.snapshot() != null) { | ||
| this.snapshot = parts.snapshot(); | ||
| } |
There was a problem hiding this comment.
So I think this branch condition is a little off (once this method also uses the path of the URL). Take this set of steps:
myClient1 = builder.endpoint("http://myaccount.blob.core.windows.net/mycontainer/myblob?snapshot=mysnapshotid").buildBlobClient();myClient2 = builder.endpoint("http://myaccount.blob.core.windows.net/anothercontainer/anotherblob").buildBlobClient();
Step 2 will NOT erase the snapshot in the builder, and myClient2 will point to http://myaccount.blob.core.windows.net/anothercontainer/anotherblob?snapshot=mysnapshotid, at best a nonexistent resource and at worst the wrong one.
We need to make decisions about what we overwrite under what circumstances when this method is used, as well as clearly document it. My first thoughts include:
- Host and scheme are always overwritten
- If any part of the path is specified, then we overwrite container name, blob name, and snapshot id. (if there is only one path segment, we assume it's the implicit root container) (we don't need to worry about only the snapshot being specified, as you can't snapshot an account)
- SAS token is always overwritten, including setting it to null if not present
Input from @rickle-msft and @JonathanGiles is much appreciated.
There was a problem hiding this comment.
I agree with those proposals, they are not only easier to explain but also removes assumptions we are trying to make on the behalf of the consumer.
JonathanGiles
left a comment
JonathanGiles
left a comment
There was a problem hiding this comment.
Approved pending the improved docs and follow-up on James' comments.
| */ | ||
| public BlobClient buildClient() { | ||
| return new BlobClient(buildAsyncClient()); | ||
| public BlobClient buildBlobClient() { |
storage/client/blob/src/main/java/com/azure/storage/blob/BlobClientBuilder.java
Outdated
Show resolved
Hide resolved
storage/client/blob/src/main/java/com/azure/storage/blob/BlobClientBuilder.java
Outdated
Show resolved
Hide resolved
* Storage SAS implementation (#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (#4375) Removes RawClients from Storage Blobs * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (#4383) * Initial check in for storage queue * Initial checkin for Storage file (#4414) * Finished the restructure, refactor builder. Added sleep in record feature. Linting * Merge Storage Blob Client Builders (#4468) Merges AppendBlobClientBuilder, BlobClientBuilder, BlockBlobClientBuilder, and PageBlobClientBuilder into a single builder class BlobClientBuilder. Additionally, JavaDoc comments for the other builder classes, ContainerClientBuilder and StorageAccountClientBuilder, were cleaned up and the way the endpoint is handled in builders was changed.
* Storage SAS implementation (#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (#4375) Removes RawClients from Storage Blobs * Add storage swaggers * Update Storage to azure-core preview 3 * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (#4383) * Initial check in for storage queue * Address code review feedback in AutoRest * Initial checkin for Storage file (#4414) * Finished the restructure, refactor builder. Added sleep in record feature. Linting * Merge Storage Blob Client Builders (#4468) Merges AppendBlobClientBuilder, BlobClientBuilder, BlockBlobClientBuilder, and PageBlobClientBuilder into a single builder class BlobClientBuilder. Additionally, JavaDoc comments for the other builder classes, ContainerClientBuilder and StorageAccountClientBuilder, were cleaned up and the way the endpoint is handled in builders was changed. * Regenerate blob file queue after merge * Up azure core version for file & queue * Apply all the manual fixes in swagger * Change directoryName to directoryPath * Use non-fluent pattern for generated clients * Fix build after merging with master * Return void on setters * Revert spotbugs config change
5 participants
Merges the Storage Blob client builders into a single builder class which is able to instantiate all blob client types.