Implement unexpectlengthexception in azure core #5108

sima-zhu · 2019-08-26T17:57:56Z

Make changes in azure core. Added tests in storage.

Will make up more tests in azure core unit tests.

…into unexpectedException

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RestProxy.java

.../azure-core/src/main/java/com/azure/core/implementation/annotation/ServiceClientBuilder.java

…into unexpectedException

alzimmermsft · 2019-08-27T17:06:38Z

Could you turn on the unit tests in Blobs that use UnexpectedLengthException and make sure those pass with this change.

jaschrep-msft · 2019-08-27T17:13:43Z

sdk/storage/azure-storage-file/pom.xml

-
+    <testResources>
+      <testResource>
+        <directory>${basedir}/src/test/resources</directory>


What is this doing? I'm not super familiar with maven, but we don't have this in the blob pom even though we have the same folders in that project.

This is how groovy use the test resource folder. I did not see any file operation in blob. @alzimmermsft do you know how you do this in blob groovy test?

Blobs doesn't use any file resources as part of testing, aside from the session-records which the playback framework handles the logic for that. Blobs generates and tears down file creation for the tests that need it.

jaschrep-msft · 2019-08-27T17:15:38Z

sdk/storage/azure-storage-file/src/test/java/com/azure/storage/file/spock/APISpec.groovy

@@ -18,12 +21,14 @@ import com.azure.storage.file.ShareClientBuilder
 import com.azure.storage.file.models.ListSharesOptions
 import spock.lang.Specification

+import java.util.function.Supplier


Why are all these imports added when all you did is remove a line?

jaschrep-msft · 2019-08-27T17:28:23Z

sdk/storage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileAPITests.groovy

@@ -195,6 +191,23 @@ class FileAPITests extends APISpec {
        FileTestHelper.assertExceptionStatusCodeAndMessage(e, 416, StorageErrorCode.INVALID_RANGE)
    }

+    @Unroll
+    def "Upload data length mismatch"() {


Nit: can you put a blank line before each block label so it's easier to read?

Thanks! A little annoying if I have this in this PR. Will do in the issue.
#5066

jaschrep-msft · 2019-08-27T17:34:17Z

sdk/storage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileAPITests.groovy

+        where:
+        size | errMsg
+        6 | "more bytes than"
+        8 | "less bytes than"


Nit: align.

jaschrep-msft · 2019-08-27T17:36:37Z

...orage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileAsyncAPITests.groovy

@@ -141,6 +142,26 @@ class FileAsyncAPITests extends APISpec {
        defaultData.clear()
    }

+    @Unroll
+    def "Upload data length mismatch"() {


Same comments as on the sync test.

jaschrep-msft · 2019-08-27T17:38:29Z

...orage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileAsyncAPITests.groovy

-        File downloadFile = new File(testFolder.getPath() + "/testDownload")
+        def testFolder = getClass().getClassLoader().getResource("testfiles")
+        File uploadFile = new File(testFolder.getPath(), "/helloworld")
+        File downloadFile = new File(testFolder.getPath(), "/testDownload")


Nit: Here you use "/helloworld" and in the sync tests you use "helloworld". They should both be the same but we may as well be consistent. Since the second argument is meant to be the file name, I'd say remove the slash here.

jaschrep-msft · 2019-08-27T17:42:59Z

sdk/storage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileTestHelper.groovy

@@ -202,10 +204,15 @@ class FileTestHelper {

    static String createRandomFileWithLength(int size, String folder, String fileName) {
        def path = Paths.get(folder)
+        if (path == null) {
+            logger.logExceptionAsError("The folder path does not exist.")


This can't work. The method accepts a RuntimeException and you pass it a string.

Good catch. Fixed

jaschrep-msft · 2019-08-27T17:43:36Z

sdk/storage/azure-storage-file/src/test/java/com/azure/storage/file/spock/FileTestHelper.groovy

@@ -202,10 +204,15 @@ class FileTestHelper {

    static String createRandomFileWithLength(int size, String folder, String fileName) {
        def path = Paths.get(folder)
+        if (path == null) {
+            logger.logExceptionAsError("The folder path does not exist.")
+            return null


If we fail to make a file, I'd rather throw with a useful message instead of getting a null ref exception later in the test.

Fixed and throw

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RestProxy.java

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlockBlobClient.java

...torage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceClientBuilder.java

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/AppendBlobAPITest.groovy

sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/Utility.java

jaschrep-msft

A couple reversions to make and a question that may involve moving some stuff.

jaschrep-msft · 2019-09-03T17:35:18Z

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/AppendBlobAPITest.groovy

@@ -325,6 +325,8 @@ class AppendBlobAPITest extends APISpec {
        ByteArrayOutputStream downloadStream = new ByteArrayOutputStream(1024)
        destURL.download(downloadStream)
        downloadStream.toByteArray() == Arrays.copyOfRange(data, 2 * 1024, 3 * 1024)
+        cleanup:
+        defaultInputStream.get().reset()


defaultInputStream is a Supplier<InputStream> that news up a stream of the bytes of a preset string and just gives us that. These cleanup blocks are just getting a brand new stream only to call reset() and discard the reference for the GC to pick up.

Revert it back.

jaschrep-msft · 2019-09-03T17:45:30Z

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/BlobAPITest.groovy

@@ -50,7 +51,7 @@ class BlobAPITest extends APISpec {
        HttpHeaders headers = response.headers()

        then:
-        body == defaultData
+        defaultData.compareTo(body) == 0


This was already idiomatic to groovy. == checks to see if Comparable is implemented and calls objA.compareTo(objB) == 0 if so. Otherwise, it calls objA.equals(objB).

(future reference: if you want to compare reference equality in groovy, you write objA.is(objB))

Revert the changes back. Thanks for the info!

jaschrep-msft · 2019-09-03T17:52:24Z

sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/Utility.java

+     * @throws UnexpectedLengthException when input data length mismatch input length.
+     * @throws RuntimeException When I/O error occurs.
+     */
+    public static Flux<ByteBuffer> convertStreamToByteBuffer(InputStream data, long length, int blockSize) {


I feel like this is a common enough case that this should be in core? We can't be the only library moving data and need to do this conversion.

Will discuss with @JonathanGiles if we can put up the helper method in Azure core. Leave it here right now as only storage has this kind of needs.

Double check the Utility methods. There are bunch a few which do not specific to storage. We can several of them into Azure core based on the needs of other clients. It is better to limit the scope of the utility to storage before preview 3.

alzimmermsft

Just re-run the failed test step, MacOS has a transient test failure due to timing issues.

JonathanGiles · 2019-09-03T23:37:45Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/AppendBlobClient.java

@@ -159,24 +161,14 @@ public AppendBlobItem appendBlock(InputStream data, long length) {
     * @param context Additional context that is passed through the Http pipeline during the service call.
     *
     * @return A {@link Response} whose {@link Response#value() value} contains the append blob operation.
+     * @throws UnexpectedLengthException when the length of data does not match the input {@code length}.


You cannot throw UnexpectedLengthException here as it is not public API. You either need to make it public API, or not throw it from public API.

Put the exception in public exception folder

…into unexpectedException

anuchandy · 2019-09-04T16:12:37Z

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RestProxy.java

+
+        return Flux.defer(() -> {
+            Long expectedLength = Long.valueOf(request.headers().value("Content-Length"));
+            final long[] currentTotalLength = new long[1];


I guess what we're doing here is correct. I asked the question in SOF and got a response from David (the main contributor of rx) - https://stackoverflow.com/questions/57776012/mutating-array-elements-in-reactive-publisher-onnext-doonnext-methods/
The last thing to rule out is whether to use volatile keyword to ensure immediate update visibility across thread, waiting for his response.

we're good, David confirmed that framework takes care of memory visibility, no need for volatile. https://stackoverflow.com/questions/57776012/mutating-array-elements-in-reactive-publisher-onnext-doonnext-methods/57786588?noredirect=1#comment102032427_57786588

Thanks Anu for taking care of this! Learned a lot from the thread.

mssfang · 2019-09-04T18:14:40Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/PageBlobClient.java

-                return ByteBuffer.wrap(cache);
-            }));
+        Objects.requireNonNull(body);
+        long length = pageRange.end() - pageRange.start() + 1;


nit: final modifier for unmodifiable variable.

…into unexpectedException

Implement unexpectlengthexception in azure core

8a3e7dc

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

sima-zhu requested review from alzimmermsft, gapra-msft, hemanttanwar, jaschrep-msft, jianghaolu, rickle-msft and srnagar as code owners August 26, 2019 17:57

sima-zhu self-assigned this Aug 26, 2019

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

1d9f1cf

…into unexpectedException

alzimmermsft reviewed Aug 26, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RestProxy.java Outdated Show resolved Hide resolved

alzimmermsft reviewed Aug 26, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RestProxy.java Outdated Show resolved Hide resolved

alzimmermsft reviewed Aug 26, 2019

View reviewed changes

.../azure-core/src/main/java/com/azure/core/implementation/annotation/ServiceClientBuilder.java Outdated Show resolved Hide resolved

sima-zhu added 6 commits August 26, 2019 13:36

merge from mainline

fffb585

Remove unnessary files.

89039fc

Added more playback jsons

ee165be

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

d3d8c04

…into unexpectedException

Push more changes to fix bytebuffer and file creation

96f3b79

Added more sync and async tests.

623d6e7

sima-zhu requested a review from alzimmermsft August 27, 2019 00:57

Remove extra blocks

43ab40e

jaschrep-msft requested changes Aug 27, 2019

View reviewed changes

sima-zhu added 2 commits August 28, 2019 13:06

enable blob

d162cd1

Make changes to lazy verifing the length

c71ceb4

alzimmermsft requested changes Aug 30, 2019

View reviewed changes

sima-zhu added 2 commits August 30, 2019 14:53

add unexpectedlengthexception.

0666ca4

Removed the one in blob.

823999d

Added blank lines.

55a1b53

srnagar approved these changes Sep 3, 2019

View reviewed changes

jaschrep-msft requested changes Sep 3, 2019

View reviewed changes

Reverted some changes in blob.

7a802f5

sima-zhu requested review from alzimmermsft, srnagar and jaschrep-msft September 3, 2019 18:15

sima-zhu added 3 commits September 3, 2019 11:21

Use logger in helper methods.

165a0b2

Remove extra lines

c63f931

Clean up more tests.

f599930

alzimmermsft approved these changes Sep 3, 2019

View reviewed changes

jaschrep-msft approved these changes Sep 3, 2019

View reviewed changes

JonathanGiles requested changes Sep 3, 2019

View reviewed changes

sima-zhu added 4 commits September 3, 2019 17:01

Move exception to public exception folder.

3c34beb

Make exception into public.

dd1dbab

Fixed javadoc complaint

b88014e

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

942d7bc

…into unexpectedException

sima-zhu requested a review from JonathanGiles September 4, 2019 00:58

Addressed JavaDoc issue.

9a9ed7c

JonathanGiles approved these changes Sep 4, 2019

View reviewed changes

anuchandy reviewed Sep 4, 2019

View reviewed changes

sima-zhu added 2 commits September 4, 2019 09:39

merge from mainline

9ee4bb7

Fixed json after merge

78fff12

mssfang reviewed Sep 4, 2019

View reviewed changes

sima-zhu added 4 commits September 4, 2019 12:11

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

5e16f5a

…into unexpectedException

Added final

645da4a

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

43b5581

…into unexpectedException

Added playback jsons

8a38331

sima-zhu merged commit f440dd6 into Azure:master Sep 5, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unexpectlengthexception in azure core #5108

Implement unexpectlengthexception in azure core #5108

sima-zhu commented Aug 26, 2019

alzimmermsft commented Aug 27, 2019

jaschrep-msft Aug 27, 2019

sima-zhu Aug 30, 2019

alzimmermsft Aug 30, 2019

jaschrep-msft Aug 27, 2019

jaschrep-msft Aug 27, 2019

sima-zhu Aug 30, 2019

jaschrep-msft Aug 27, 2019

jaschrep-msft Aug 27, 2019

sima-zhu Aug 30, 2019

jaschrep-msft Aug 27, 2019

jaschrep-msft Aug 27, 2019

sima-zhu Aug 30, 2019

jaschrep-msft Aug 27, 2019

sima-zhu Aug 30, 2019

jaschrep-msft left a comment

jaschrep-msft Sep 3, 2019

sima-zhu Sep 3, 2019

jaschrep-msft Sep 3, 2019

sima-zhu Sep 3, 2019

jaschrep-msft Sep 3, 2019

sima-zhu Sep 3, 2019

sima-zhu Sep 3, 2019

alzimmermsft left a comment

JonathanGiles Sep 3, 2019

sima-zhu Sep 3, 2019

anuchandy Sep 4, 2019

anuchandy Sep 5, 2019

sima-zhu Sep 5, 2019

mssfang Sep 4, 2019

Implement unexpectlengthexception in azure core #5108

Implement unexpectlengthexception in azure core #5108

Conversation

sima-zhu commented Aug 26, 2019

alzimmermsft commented Aug 27, 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

jaschrep-msft 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

alzimmermsft 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