Initial checkin for Storage file #4414

sima-zhu · 2019-07-12T17:33:45Z

Linting, Junit test, builder refactor

…ture. Linting

alzimmermsft

General question around the large model classes that were created, should they be simplified to only contain the most important pieces of information related to the service. If so remove the common HTTP headers as that information will still be available using the Response class headers.

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

storage/client/file/src/main/java/com/azure/storage/file/DirectoryClient.java

storage/client/file/src/main/java/com/azure/storage/file/DirectoryClientBuilder.java

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

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

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

sima-zhu · 2019-07-15T17:30:03Z

General question around the large model classes that were created, should they be simplified to only contain the most important pieces of information related to the service. If so remove the common HTTP headers as that information will still be available using the Response class headers.

I have simplified the model class. Etag is the essential information we want to return.
I have get rid of the date, request id, and error part. Which field you think is also unnessary?

…for-java into storage-file

jaschrep-msft · 2019-07-18T19:51:06Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @param directoryName Name of the directory
+     * @param snapshot The snapshot of the share
+     */
+    DirectoryAsyncClient(AzureFileStorageImpl azureFileStorageClient, String shareName, String directoryName, String snapshot) {


Why do all of our constructors create their own builders? This leads us to a code path where we create a builder to pass all the relevant info to the client which creates a new builder with the same info to compile the relevant info correctly for the client. We can probably remove these extra steps.

Sima - I still see the builder code that James is calling out. What has been done here?

I did not push the commit at the time you reviewed. It is there right now. Could you check?

jaschrep-msft · 2019-07-18T19:59:48Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

    }

+    /**
+     * Creates a directory in the storage account and returns a response of {@link DirectoryInfo} to interact with it.


Convey that it creates the directory this client corresponds to, not just a directory. Also, the directory is technically created in the share. Two identical directory paths can exist in different shares. Perhaps Creates a directory in the storage account --> Creates this directory in the file share.

jaschrep-msft · 2019-07-18T20:55:35Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

-    public Mono<Response<DirectoryInfo>> setMetadata(Map<String, String> metadata) {
-        throw new UnsupportedOperationException();
+    /**
+     * Lists all directories and files in the storage account without their prefix or maxResult.


Lists all directories and files in the storage account --> Lists all sub-directories and files in this directory
There are several instances where you scope the operation to the account, when it's really just to the contents of this directory. Be sure to address them. Additionally, double check the sync DirectoryClient for these issues as well.

jaschrep-msft · 2019-07-18T21:24:47Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @param recursive Specifies operation should apply to the directory specified in the URI, its files, its subdirectories and their files.
+     * @return {@link HandleItem handles} in the directory that satisfy the requirements
+     */
+    public Flux<HandleItem> getHandles(Integer maxResult, boolean recursive) {


maxResult -> pageSize
While it's technically maxResult in REST, that's per paged request.

I'd prefer to address this across all storage SDK

jaschrep-msft · 2019-07-18T21:34:39Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @param handleId Specifies the handle ID to be closed. Use an asterisk ('*') as a wildcard string to specify all handles.
+     * @param recursive A boolean value that specifies if the operation should also apply to the files and subdirectories of the directory specified in the URI.
+     * @return The counts of number of handles closed
+     */
    public Flux<Integer> forceCloseHandles(String handleId, boolean recursive) {


This is technically correct according to the REST layer, but its usage is a bit flexible. For instance, handleId can be either the name of the handle to close, or *, meaning close all handles. Additionally, recursive doesn't make much sense if we've provided the specific handleId to close, and the REST endpoint actually has undefined behavior (unless they patched this) when both the wildcard and recursive=false are specified.

The way we handled this in track 1 was by exposing two different methods: forceCloseHandle(String handleId) and forceCloseAllHandles(boolean recursive), supplying the appropriate missing parameter. This allowed us to better guide users to what is expected use.

Issue created.

If an issue is created, link to if when you reference it so other interested people can find it and subscribe / offer comments.

Linked the issue in Java doc

jaschrep-msft · 2019-07-18T21:46:15Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @return A response containing the subdirectory client and the status of creating the directory.
+     * @throws StorageErrorException If the subdirectory has already existed, the parent directory does not exist or directory is an invalid resource name.
+     */
+    public Mono<Response<DirectoryAsyncClient>> createSubDirectory(String subDirectoryName) {


Have this method call this.createSubDirectory(subDirectoryName, null). Same pattern we follow for overloads of service calls.

Similar comment for createFile().

jaschrep-msft · 2019-07-18T21:46:42Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     */
+    public Mono<Response<DirectoryAsyncClient>> createSubDirectory(String subDirectoryName, Map<String, String> metadata) {
+        String directoryPath = directoryName + "/" + subDirectoryName;
+        DirectoryAsyncClient createSubClient = new DirectoryAsyncClient(azureFileStorageClient, shareName, directoryPath, snapshot);


Just call getSubDirectoryClient(). It's these exact lines.

Similar comment for createFile().

jaschrep-msft · 2019-07-18T22:17:27Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

    public Mono<VoidResponse> deleteFile(String fileName) {
-        throw new UnsupportedOperationException();
+        String filePath = directoryName + "/" + fileName;
+        FileAsyncClient fileAsyncClient = new FileAsyncClient(azureFileStorageClient, shareName, filePath, snapshot);


Same as before. We have methods for this logic. Just use them here and for deleting directories.

jaschrep-msft · 2019-07-18T22:25:38Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryClientBuilder.java

+        this.sasTokenCredential = credential;
+        return this;
+    }
+


Where is the credential method that takes a SharedKeyCredential?

jaschrep-msft · 2019-07-18T22:56:36Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryClientBuilder.java

+     * @param snapshot Identifier of the snapshot
+     * @return the updated DirectoryClientBuilder object.
+     */
+    public DirectoryClientBuilder snapshot(String snapshot) {


We should probably specify that this is a snapshot of the file share, not the directory.

JonathanGiles

I'm seeing a lot of "done" comments that don't have the changes showing. I imagine this is because they haven't been pushed to GitHub. Please push the fixes, and consider my feedback, and then I will review again.

JonathanGiles · 2019-07-23T02:37:43Z

storage/client/file/README.md

+
+```Java
+fileClient.setHttpHeaders(httpHeaders);
+```


Will all these code samples be fleshed out? If not, I don't think they provide any value as single-line code samples.

I will make up some necessary code.
Thanks for reminding.

It has link in description which point to the place how we build a client.

I will have more codes show how to build up the Object other than String type. e.g httpHeaders.

JonathanGiles · 2019-07-23T02:38:36Z

storage/client/file/pom.xml

+    <dependency>
+      <groupId>org.slf4j</groupId>
+      <artifactId>slf4j-api</artifactId>
+    </dependency>


slf4j-api should come in transitively and not need to be included here.

JonathanGiles · 2019-07-23T02:39:15Z

storage/client/file/pom.xml

+      <groupId>com.microsoft.azure</groupId>
+      <artifactId>adal4j</artifactId>
+      <scope>test</scope>
+    </dependency>


Same as in queues - is slf4j-simple, reactor-test, and adal4j required here?

reactor-test is necessary for async tests.

Removed adal4j , slf4j-simple

JonathanGiles · 2019-07-23T02:39:28Z

storage/client/file/src/main/java/com/azure/storage/common/credentials/SharedKeyCredential.java

+     *                              <ul>
+     *                                  <li> If the HMAC-SHA256 signature for {@code sharedKeyCredentials} fails to generate. </li>
+     *                                  <li> If the an invalid key has been given to the client. </li>
+     *                              </ul>


Move to the left.

JonathanGiles · 2019-07-23T02:39:54Z

storage/client/file/src/main/java/com/azure/storage/common/credentials/SharedKeyCredential.java

+        } catch (final NoSuchAlgorithmException e) {
+            throw new RuntimeException("There is no such algorithm. Error Details: " + e.getMessage());
+        } catch (InvalidKeyException e) {
+            throw new RuntimeException("Please double check the account key. Error details: " + e.getMessage());


Use clientLogger logAndThrow API

The log and throw API is not available to preview 2.
Issue opened. #4551

JonathanGiles · 2019-07-23T02:41:53Z

storage/client/file/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java

+     *                                        <ul>
+     *                                            <li> There is only one null value for retryDelay and maxRetryDelay.<li/>
+     *                                            <li> Unrecognized retry policy type.<li/>
+     *                                        </ul>


storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

JonathanGiles · 2019-07-23T04:26:09Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @param directoryName Name of the directory
+     * @param snapshot The snapshot of the share
+     */
+    DirectoryAsyncClient(AzureFileStorageImpl azureFileStorageClient, String shareName, String directoryName, String snapshot) {


Sima - I still see the builder code that James is calling out. What has been done here?

JonathanGiles · 2019-07-23T04:30:23Z

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     *
+     * {@codesnippet com.azure.storage.file.directoryAsyncClient.setMetadata#map.clearMetadata}
+     *
+     * @param metadata Optional. Metadata to set on the directory, if null is passed the metadata for the directory is cleared


Don't write these as "Optional. Metadata ..." - make it a full sentence, e.g. "Optional metadata to set on the directory..".
Do this in all cases where the "Optional. " approach is being used currently.

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

storage/client/file/src/main/java/com/azure/storage/file/DirectoryAsyncClient.java

+     * @param handleId Specifies the handle ID to be closed. Use an asterisk ('*') as a wildcard string to specify all handles.
+     * @param recursive A boolean value that specifies if the operation should also apply to the files and subdirectories of the directory specified in the URI.
+     * @return The counts of number of handles closed
+     */
    public Flux<Integer> forceCloseHandles(String handleId, boolean recursive) {


If an issue is created, link to if when you reference it so other interested people can find it and subscribe / offer comments.

…toryAsyncClient.java Co-Authored-By: Jonathan Giles <[email protected]>

…for-java into storage-file

alzimmermsft

Approving based on the major comments not being addressed in this PR being documented as issues.

JonathanGiles

Given the approval of Rick, James, and Alan, this PR doesn't need me to cast my eyes over it also.

JonathanGiles · 2019-07-23T23:16:55Z

eng/code-quality-reports/src/main/resources/spotbugs/spotbugs-exclude.xml

+  <Match>
+    <Class name="~com\.azure\.(.+)Impl"/>
+    <Bug pattern="NP_LOAD_OF_KNOWN_NULL_VALUE"/>
+  </Match>


Don't duplicate the pattern - put it in the Or clause above instead.

* 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

Finished the restructure, refactor builder. Added sleep in record fea…

e43a8b2

…ture. Linting

sima-zhu force-pushed the storage-file branch from b64963e to e43a8b2 Compare July 12, 2019 17:35

sima-zhu self-assigned this Jul 12, 2019

sima-zhu added Storage Storage Service (Queues, Blobs, Files) Client This issue points to a problem in the data-plane of the library. labels Jul 12, 2019

sima-zhu requested a review from alzimmermsft July 12, 2019 18:04

sima-zhu added 2 commits July 12, 2019 11:41

Fixed the macOS

b7c446b

merge from mainlnie

889ec07

sima-zhu force-pushed the storage-file branch from ddd49a6 to 889ec07 Compare July 12, 2019 18:46

sima-zhu added 3 commits July 12, 2019 12:26

Added commit for macOS and linux for debuging the filepath

8334cb1

Fixed filePath

3a6470b

Remove comments

0ae0d8a

alzimmermsft requested changes Jul 12, 2019

View reviewed changes

sima-zhu and others added 3 commits July 12, 2019 17:32

Keep the download file.

bddf4da

Added the empty download file.

ca12657

Fixed the failure test in linux and mac

cdfeb09

alzimmermsft requested a review from JonathanGiles July 15, 2019 21:19

sima-zhu added 5 commits July 16, 2019 10:53

Fixed the upload download tests.

740e7e1

Fixed the record and playback tests

456deb3

Fix checkstyle

b1ea3be

URL and snapshot refactor. FileService JavaDoc code snippet done.

7c0f87d

Fixed the failed JavaDoc

f98107e

sima-zhu requested a review from rickle-msft July 19, 2019 05:11

sima-zhu added 2 commits July 19, 2019 00:18

Share client java doc code snippet.

6f729e0

Directory client java doc code snippet.

93a5fce

sima-zhu requested review from danieljurek, mitchdenny and weshaggard as code owners July 19, 2019 19:09

file

e5f1ec7

sima-zhu and others added 7 commits July 20, 2019 01:00

file client java doc code snippet

efb304b

Merge branch 'storage-post-review1-dev' into storage-file

f6853df

CLean up the javadoc snippet

67410a8

Merge branch 'storage-file' of https://github.com/sima-zhu/azure-sdk-…

a65ed6d

…for-java into storage-file

Addressed comments

67aee2a

Addressed the comments from PR

6047a3f

Fixed README

9ceb72b

jaschrep-msft reviewed Jul 22, 2019

View reviewed changes

Addressed the comments from PR and meeting,

ddb43c1

JonathanGiles requested changes Jul 23, 2019

View reviewed changes

sima-zhu and others added 6 commits July 22, 2019 22:42

Addressed the comments from PR and meeting.

bd80931

Add file version.

b615c23

Fixed the playback test

1f83cb2

Update storage/client/file/src/main/java/com/azure/storage/file/Direc…

738677a

…toryAsyncClient.java Co-Authored-By: Jonathan Giles <[email protected]>

Make changes to javadoc and readme

d673190

Merge branch 'storage-file' of https://github.com/sima-zhu/azure-sdk-…

ca039a0

…for-java into storage-file

sima-zhu requested review from jaschrep-msft, JonathanGiles and alzimmermsft July 23, 2019 18:49

alzimmermsft approved these changes Jul 23, 2019

View reviewed changes

jaschrep-msft approved these changes Jul 23, 2019

View reviewed changes

Changed the log level to warning and make it non-static

434123e

rickle-msft approved these changes Jul 23, 2019

View reviewed changes

JonathanGiles approved these changes Jul 23, 2019

View reviewed changes

Remove the duplicate exclude rule in spotbugs

67e7fcb

sima-zhu merged commit 44fb56e into Azure:storage-post-review1-dev Jul 24, 2019

sima-zhu deleted the storage-file branch February 23, 2021 19:05

Initial checkin for Storage file #4414

Initial checkin for Storage file #4414

Conversation

sima-zhu commented Jul 12, 2019

alzimmermsft left a comment

Choose a reason for hiding this comment

sima-zhu commented Jul 15, 2019

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

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

sima-zhu Jul 23, 2019 • edited Loading

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

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

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

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

JonathanGiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sima-zhu Jul 23, 2019 •

edited

Loading