Update Storage to azure-core preview 3 #4489

jianghaolu · 2019-07-19T18:14:22Z

Migrated from #4482

alzimmermsft

Overall looks good, just a few important questions.

storage/client/blob/src/main/java/com/azure/storage/blob/implementation/AppendBlobsImpl.java

storage/client/blob/src/main/java/com/azure/storage/blob/implementation/BlobsImpl.java

storage/client/blob/src/main/java/com/azure/storage/blob/implementation/BlockBlobsImpl.java

storage/client/blob/src/main/java/com/azure/storage/blob/implementation/PageBlobsImpl.java

...lient/blob/src/main/java/com/azure/storage/blob/implementation/SignedIdentifiersWrapper.java

storage/client/blob/src/main/java/com/azure/storage/blob/models/BlobHierarchyListSegment.java

storage/client/blob/src/main/java/com/azure/storage/blob/models/package-info.java

storage/client/queue/src/main/java/com/azure/storage/queue/models/PeekedMessageItem.java

jianghaolu · 2019-07-22T17:53:22Z

Thanks for catching all the manual fixes - I'll fix up the swagger in this repo.

JonathanGiles

All @ServiceInterface annotations should have a name that is more descriptive, for when it is used with tracing, telemetry, etc.

JonathanGiles · 2019-07-23T01:43:32Z

...ge/client/blob/src/main/java/com/azure/storage/blob/implementation/AzureBlobStorageImpl.java

+     */
+    public HttpPipeline httpPipeline() {
+        return this.httpPipeline;
+    }


Even though this is not public API I'll ask anyway: is this required to be exposed? I would suggest removing it until it is required.

This is used at

azure-sdk-for-java/storage/client/blob/src/main/java/com/azure/storage/blob/implementation/AppendBlobsImpl.java

Line 61 in 593ebf2

this.service = RestProxy.create(AppendBlobsService.class, client.httpPipeline());

storage/client/blob/src/main/java/com/azure/storage/blob/implementation/ServicesImpl.java

storage/client/file/src/main/java/com/azure/storage/file/implementation/DirectorysImpl.java

storage/client/file/src/main/java/com/azure/storage/file/implementation/ServicesImpl.java

JonathanGiles · 2019-07-23T01:48:36Z

...ge/client/file/src/main/java/com/azure/storage/file/implementation/AzureFileStorageImpl.java

+     */
+    public HttpPipeline httpPipeline() {
+        return this.httpPipeline;
+    }


Remove unless required.

JonathanGiles · 2019-07-23T01:49:47Z

storage/client/file/src/main/java/com/azure/storage/file/models/DeleteSnapshotsOptionType.java

-     * Enum value only.
-     */
-    ONLY("only");
+    INCLUDE("include");


An enum with only one built-in value type? Is it required?

This is generated from Swagger.

See https://github.com/Azure/azure-sdk-for-java/blob/master/storage/client/file/swagger/file.json#L3365

JonathanGiles · 2019-07-23T01:51:24Z

storage/client/file/src/main/java/com/azure/storage/file/models/ShareProperties.java

@@ -84,7 +86,7 @@ public ShareProperties etag(String etag) {
     *
     * @return the quota value.
     */
-    public int quota() {
+    public long quota() {


I'm noticing that ShareGetPropertiesHeaders had reference types for Integer and Long, whereas this class has primitive types for int and long. Is this intentional? Can we standardise?

In the generated code, currently we are generating boxed types if the Swagger defines it as optional - we don't want to throw NullPointerException in our deserialization. The convenience layer can make intelligent defaults but that cannot be automated in the generator.

…-for-java into storage-swagger

alzimmermsft

LGTM, let's get this in to unblock some other PR! 😄

storage/client/file/src/main/java/com/azure/storage/file/implementation/FilesImpl.java

storage/client/file/src/main/java/com/azure/storage/file/implementation/DirectorysImpl.java

storage/client/file/src/main/java/com/azure/storage/file/implementation/SharesImpl.java

…-for-java into storage-swagger

sima-zhu

Ship it

sima-zhu

Can we change the FileService -> FileStorage and QueueService -> QueueStorage in auto-gen layer

* Storage SAS implementation (Azure#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (Azure#4375) Removes RawClients from Storage Blobs * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (Azure#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (Azure#4383) * Initial check in for storage queue * Initial checkin for Storage file (Azure#4414) * Finished the restructure, refactor builder. Added sleep in record feature. Linting * Merge Storage Blob Client Builders (Azure#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.

JonathanGiles · 2019-07-26T00:26:07Z

...ge/client/blob/src/main/java/com/azure/storage/blob/implementation/AzureBlobStorageImpl.java

@@ -32,7 +31,7 @@ public String url() {
     * @param url the url value.
     * @return the service client itself.
     */
-    AzureBlobStorageImpl url(String url) {
+    AzureBlobStorageImpl setUrl(String url) {


Because this is no longer fluent, we should no longer return this.

…ate serializer directly with the JacksonAdapter. Tests that failed after functionality was removed from HttpHeaders now pass with the new HttpHeaderSerializer. (Azure#4597)

…Azure#4589) * add MethodName rule to disable builder as a method name and update issues

…-for-java into storage-swagger

…rage-swagger

jianghaolu · 2019-07-26T21:03:31Z

Moving back to #4482.

jianghaolu added 2 commits July 17, 2019 14:22

Add storage swaggers

10a85ad

Update Storage to azure-core preview 3

593ebf2

jianghaolu mentioned this pull request Jul 19, 2019

Update Storage to azure-core preview 3 #4482

Merged

jianghaolu requested review from alzimmermsft, sima-zhu and JonathanGiles July 19, 2019 18:14

alzimmermsft requested changes Jul 19, 2019

View reviewed changes

JonathanGiles requested changes Jul 23, 2019

View reviewed changes

jianghaolu added 3 commits July 23, 2019 17:14

Address code review feedback in AutoRest

b602214

Merge branch 'storage-post-review1-dev' of github.com:Azure/azure-sdk…

9cbf411

…-for-java into storage-swagger

Merge branch 'storage-post-review1-dev' of github.com:Azure/azure-sdk…

822b41d

…-for-java into storage-swagger

alzimmermsft mentioned this pull request Jul 24, 2019

Changes for javadoc and logger #4552

Closed

alzimmermsft self-requested a review July 24, 2019 16:52

alzimmermsft approved these changes Jul 24, 2019

View reviewed changes

sima-zhu reviewed Jul 24, 2019

View reviewed changes

storage/client/file/src/main/java/com/azure/storage/file/implementation/FilesImpl.java Outdated Show resolved Hide resolved

sima-zhu reviewed Jul 24, 2019

View reviewed changes

storage/client/file/src/main/java/com/azure/storage/file/implementation/FilesImpl.java Outdated Show resolved Hide resolved

sima-zhu reviewed Jul 24, 2019

View reviewed changes

storage/client/file/src/main/java/com/azure/storage/file/implementation/DirectorysImpl.java Outdated Show resolved Hide resolved

sima-zhu reviewed Jul 24, 2019

View reviewed changes

storage/client/file/src/main/java/com/azure/storage/file/implementation/SharesImpl.java Outdated Show resolved Hide resolved

jianghaolu added 5 commits July 24, 2019 10:23

Regenerate blob file queue after merge

e19ea2b

Up azure core version for file & queue

7149824

Apply all the manual fixes in swagger

efc335c

Change directoryName to directoryPath

87f3090

Merge branch 'storage-post-review1-dev' of github.com:Azure/azure-sdk…

5ce9473

…-for-java into storage-swagger

jianghaolu requested a review from rickle-msft July 24, 2019 21:33

Use non-fluent pattern for generated clients

28beb79

sima-zhu approved these changes Jul 24, 2019

View reviewed changes

sima-zhu reviewed Jul 25, 2019

View reviewed changes

JonathanGiles reviewed Jul 26, 2019

View reviewed changes

JonathanGiles and others added 6 commits July 26, 2019 13:10

Removing the Jackson leak in HttpHeaders, instead registering a separ…

ea2f43a

…ate serializer directly with the JacksonAdapter. Tests that failed after functionality was removed from HttpHeaders now pass with the new HttpHeaderSerializer. (Azure#4597)

Batch #2 - 11. No .builder() API: No classes should have a .builder() (…

9739dc6

…Azure#4589) * add MethodName rule to disable builder as a method name and update issues

Add Tracing context in Key Vault keys package (Azure#4500)

dd272bf

Merge branch 'storage-post-review1-dev' of github.com:Azure/azure-sdk…

62c2dfc

…-for-java into storage-swagger

Merge branch 'master' of github.com:Azure/azure-sdk-for-java into sto…

a5bc2dd

…rage-swagger

Fix build after merging with master

a2b67ea

jianghaolu requested review from conniey, danieljurek, g2vinay, hemanttanwar, jaschrep-msft, mitchdenny, samvaity, srnagar and weshaggard as code owners July 26, 2019 20:46

jianghaolu closed this Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Storage to azure-core preview 3 #4489

Update Storage to azure-core preview 3 #4489

jianghaolu commented Jul 19, 2019

alzimmermsft left a comment

jianghaolu commented Jul 22, 2019

JonathanGiles left a comment

JonathanGiles Jul 23, 2019

jianghaolu Jul 23, 2019

JonathanGiles Jul 23, 2019

JonathanGiles Jul 23, 2019

jianghaolu Jul 23, 2019

jianghaolu Jul 23, 2019

JonathanGiles Jul 23, 2019

jianghaolu Jul 23, 2019

alzimmermsft left a comment

sima-zhu left a comment

sima-zhu left a comment

JonathanGiles Jul 26, 2019

jianghaolu commented Jul 26, 2019

Update Storage to azure-core preview 3 #4489

Update Storage to azure-core preview 3 #4489

Conversation

jianghaolu commented Jul 19, 2019

alzimmermsft left a comment

Choose a reason for hiding this comment

jianghaolu commented Jul 22, 2019

JonathanGiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft left a comment

Choose a reason for hiding this comment

sima-zhu left a comment

Choose a reason for hiding this comment

sima-zhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianghaolu commented Jul 26, 2019