Added Custom Deserializers to Handle Interspersed List Responses

eng/code-quality-reports/src/main/resources/revapi/revapi.json

@@ -241,6 +241,11 @@
         "old": "class com.azure.messaging.eventhubs.EventHubClientBuilder",
         "new": "class com.azure.messaging.eventhubs.EventHubClientBuilder",
         "justification": "Setting protocol to AMQP in @ServiceClientBuilder annotation is not a breaking change"
+      },
+      {
+        "code": "java.annotation.added",
+        "new": "class com.azure.storage.blob.models.PageList",
+        "justification": "Annotation required to resolve deserialization bug."
       }
     ]
   }

sdk/storage/azure-storage-blob/CHANGELOG.md

@@ -1,7 +1,7 @@
 # Release History
 
 ## 12.9.0-beta.3 (Unreleased)
-
+- Fixed a bug where interspersed element types returned by page listing would deserialize incorrectly.
 
 ## 12.9.0-beta.2 (2020-10-08)
 - Added support to specify whether or not a pipeline policy should be added per call or per retry.

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/models/PageList.java

@@ -6,7 +6,9 @@
 
 import com.azure.core.annotation.Fluent;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -15,6 +17,7 @@
  */
 @JacksonXmlRootElement(localName = "PageList")
 @Fluent
+@JsonDeserialize(using = PageListDeserializer.class)
 public final class PageList {
     /*
      * The pageRange property.

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/models/PageListDeserializer.java

@@ -0,0 +1,73 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.storage.blob.models;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.core.JsonTokenId;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+
+import java.io.IOException;
+import java.util.ArrayList;
+
+/**
+ * Custom Jackson JsonDeserializer that handles deserializing PageList responses.
+ * <p>
+ * PageList responses intersperse PageRange and ClearRange elements, without this deserializer if we received the
+ * following response the resulting PageList would only contain one PageRange element and one ClearRange element.
+ *
+ * <pre>
+ * {@code
+ * <?xml version="1.0" encoding="utf-8"?>
+ * <PageList>
+ *    <PageRange>
+ *       <Start>Start Byte</Start>
+ *       <End>End Byte</End>
+ *    </PageRange>
+ *    <ClearRange>
+ *       <Start>Start Byte</Start>
+ *       <End>End Byte</End>
+ *    </ClearRange>
+ *    <PageRange>
+ *       <Start>Start Byte</Start>
+ *       <End>End Byte</End>
+ *    </PageRange>
+ * </PageList>
+ * }
+ * </pre>
+ *
+ * With the custom deserializer the response correctly returns two PageRange elements and one ClearRange element.
+ */
+final class PageListDeserializer extends JsonDeserializer<PageList> {
+    @Override
+    public PageList deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
+        ArrayList<PageRange> pageRanges = new ArrayList<>();
+        ArrayList<ClearRange> clearRanges = new ArrayList<>();
+
+        // Get the deserializer that handles PageRange.
+        JsonDeserializer<Object> pageRangeDeserializer =
+            ctxt.findRootValueDeserializer(ctxt.constructType(PageRange.class));
+
+        // Get the deserializer that handles ClearRange.
+        JsonDeserializer<Object> clearRangeDeserializer =
+            ctxt.findRootValueDeserializer(ctxt.constructType(ClearRange.class));
+
+        for (JsonToken currentToken = p.nextToken(); currentToken.id() != JsonTokenId.ID_END_OBJECT;
+             currentToken = p.nextToken()) {
+            // Get to the root element of the next item.
+            p.nextToken();
+
+            if (p.getCurrentName().equals("PageRange")) {
+                // Current token is the node that begins a PageRange object.
+                pageRanges.add((PageRange) pageRangeDeserializer.deserialize(p, ctxt));
+            } else if (p.getCurrentName().equals("ClearRange")) {
+                // Current token is the node that begins a ClearRange object.
+                clearRanges.add((ClearRange) clearRangeDeserializer.deserialize(p, ctxt));
+            }
+        }
+
+        return new PageList().setPageRange(pageRanges).setClearRange(clearRanges);
+    }
+}

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

@@ -3,11 +3,14 @@
 
 package com.azure.storage.blob.specialized
 
+import com.azure.core.util.serializer.JacksonAdapter
+import com.azure.core.util.serializer.SerializerEncoding
 import com.azure.storage.blob.APISpec
 import com.azure.storage.blob.BlobContainerAsyncClient
 import com.azure.storage.blob.BlobUrlParts
 import com.azure.storage.blob.implementation.util.BlobSasImplUtil
 import com.azure.storage.blob.models.BlobRange
+import com.azure.storage.blob.models.PageList
 import com.azure.storage.blob.sas.BlobSasPermission
 import com.azure.storage.blob.sas.BlobSasServiceVersion
 import com.azure.storage.blob.sas.BlobServiceSasSignatureValues
@@ -142,4 +145,30 @@ class HelperTest extends APISpec {
             .assertNext(){buffer -> assert buffer.compareTo(ByteBuffer.wrap(data)) == 0 }
             .verifyComplete()
     }
+
+    def "PageList custom deserializer"() {
+        setup:
+        def responseXml = "<?xml version=\"1.0\" encoding=\"utf-8\"?>  \n" +
+            "<PageList>  \n" +
+            "   <PageRange>  \n" +
+            "      <Start>0</Start>  \n" +
+            "      <End>511</End>  \n" +
+            "   </PageRange>  \n" +
+            "   <ClearRange>  \n" +
+            "      <Start>512</Start>  \n" +
+            "      <End>1023</End>  \n" +
+            "   </ClearRange>  \n" +
+            "   <PageRange>  \n" +
+            "      <Start>1024</Start>  \n" +
+            "      <End>2047</End>  \n" +
+            "   </PageRange>  \n" +
+            "</PageList>"
+
+        when:
+        def pageList = (PageList) new JacksonAdapter().deserialize(responseXml, PageList.class, SerializerEncoding.XML)
+
+        then:
+        pageList.getPageRange().size() == 2
+        pageList.getClearRange().size() == 1
+    }
 }

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

@@ -4,22 +4,25 @@
 package com.azure.storage.blob.specialized
 
 import com.azure.core.exception.UnexpectedLengthException
+import com.azure.core.util.CoreUtils
 import com.azure.storage.blob.APISpec
 import com.azure.storage.blob.BlobContainerClient
-import com.azure.storage.blob.models.PageBlobCopyIncrementalRequestConditions
 import com.azure.storage.blob.models.BlobErrorCode
 import com.azure.storage.blob.models.BlobHttpHeaders
 import com.azure.storage.blob.models.BlobRange
 import com.azure.storage.blob.models.BlobRequestConditions
 import com.azure.storage.blob.models.BlobStorageException
+import com.azure.storage.blob.models.ClearRange
 import com.azure.storage.blob.models.CopyStatusType
-import com.azure.storage.blob.options.BlobGetTagsOptions
-import com.azure.storage.blob.options.PageBlobCopyIncrementalOptions
-import com.azure.storage.blob.options.PageBlobCreateOptions
+import com.azure.storage.blob.models.PageBlobCopyIncrementalRequestConditions
 import com.azure.storage.blob.models.PageBlobRequestConditions
 import com.azure.storage.blob.models.PageRange
 import com.azure.storage.blob.models.PublicAccessType
 import com.azure.storage.blob.models.SequenceNumberActionType
+import com.azure.storage.blob.options.BlobGetTagsOptions
+import com.azure.storage.blob.options.PageBlobCopyIncrementalOptions
+import com.azure.storage.blob.options.PageBlobCreateOptions
+import com.azure.storage.common.implementation.Constants
 import spock.lang.Ignore
 import spock.lang.Unroll
 
@@ -196,7 +199,7 @@ class PageBlobAPITest extends APISpec {
         null     | null       | garbageEtag | null         | null           | null
         null     | null       | null        | receivedEtag | null           | null
         null     | null       | null        | null         | garbageLeaseID | null
-        null     | null       | null         | null        | null           | "\"notfoo\" = 'notbar'"
+        null     | null       | null        | null         | null           | "\"notfoo\" = 'notbar'"
     }
 
     def "Create error"() {
@@ -756,7 +759,7 @@ class PageBlobAPITest extends APISpec {
         null     | null       | garbageEtag | null         | null           | null
         null     | null       | null        | receivedEtag | null           | null
         null     | null       | null        | null         | garbageLeaseID | null
-        null     | null       | null         | null        | null           | "\"notfoo\" = 'notbar'"
+        null     | null       | null        | null         | null           | "\"notfoo\" = 'notbar'"
     }
 
     def "Get page ranges error"() {
@@ -770,32 +773,81 @@ class PageBlobAPITest extends APISpec {
         thrown(BlobStorageException)
     }
 
+    @Unroll
     def "Get page ranges diff"() {
         setup:
-        bc.create(PageBlobClient.PAGE_BYTES * 2, true)
+        bc.create(4 * Constants.MB, true)
 
-        bc.uploadPages(new PageRange().setStart(PageBlobClient.PAGE_BYTES).setEnd(PageBlobClient.PAGE_BYTES * 2 - 1),
-            new ByteArrayInputStream(getRandomByteArray(PageBlobClient.PAGE_BYTES)))
+        bc.uploadPages(new PageRange().setStart(0).setEnd(4 * Constants.MB - 1),
+            new ByteArrayInputStream(getRandomByteArray(4 * Constants.MB)))
 
         def snapId = bc.createSnapshot().getSnapshotId()
 
-        bc.uploadPages(new PageRange().setStart(0).setEnd(PageBlobClient.PAGE_BYTES - 1),
-            new ByteArrayInputStream(getRandomByteArray(PageBlobClient.PAGE_BYTES)))
+        rangesToUpdate.forEach({
+            bc.uploadPages(it, new ByteArrayInputStream(getRandomByteArray((int) (it.getEnd() - it.getStart()) + 1)))
+        })
 
-        bc.clearPages(new PageRange().setStart(PageBlobClient.PAGE_BYTES).setEnd(PageBlobClient.PAGE_BYTES * 2 - 1))
+        rangesToClear.forEach({ bc.clearPages(it) })
 
         when:
-        def response = bc.getPageRangesDiffWithResponse(new BlobRange(0, PageBlobClient.PAGE_BYTES * 2), snapId, null, null, null)
+        def response = bc.getPageRangesDiffWithResponse(new BlobRange(0, 4 * Constants.MB), snapId, null, null, null)
 
         then:
-        response.getValue().getPageRange().size() == 1
-        response.getValue().getPageRange().get(0).getStart() == 0
-        response.getValue().getPageRange().get(0).getEnd() == PageBlobClient.PAGE_BYTES - 1
-        response.getValue().getClearRange().size() == 1
-        response.getValue().getClearRange().get(0).getStart() == PageBlobClient.PAGE_BYTES
-        response.getValue().getClearRange().get(0).getEnd() == PageBlobClient.PAGE_BYTES * 2 - 1
         validateBasicHeaders(response.getHeaders())
-        Integer.parseInt(response.getHeaders().getValue("x-ms-blob-content-length")) == PageBlobClient.PAGE_BYTES * 2
+        response.getValue().getPageRange().size() == expectedPageRanges.size()
+        response.getValue().getClearRange().size() == expectedClearRanges.size()
+
+        for (def i = 0; i < expectedPageRanges.size(); i++) {
+            def actualRange = response.getValue().getPageRange().get(i)
+            def expectedRange = expectedPageRanges.get(i)
+            expectedRange.getStart() == actualRange.getStart()
+            expectedRange.getEnd() == actualRange.getEnd()
+        }
+
+        for (def i = 0; i < expectedClearRanges.size(); i++) {
+            def actualRange = response.getValue().getClearRange().get(i)
+            def expectedRange = expectedClearRanges.get(i)
+            expectedRange.getStart() == actualRange.getStart()
+            expectedRange.getEnd() == actualRange.getEnd()
+        }
+
+        Integer.parseInt(response.getHeaders().getValue("x-ms-blob-content-length")) == 4 * Constants.MB
+
+        where:
+        rangesToUpdate                       | rangesToClear                           | expectedPageRanges                   | expectedClearRanges
+        createPageRanges()                   | createPageRanges()                      | createPageRanges()                   | createClearRanges()
+        createPageRanges(0, 511)             | createPageRanges()                      | createPageRanges(0, 511)             | createClearRanges()
+        createPageRanges()                   | createPageRanges(0, 511)                | createPageRanges()                   | createClearRanges(0, 511)
+        createPageRanges(0, 511)             | createPageRanges(512, 1023)             | createPageRanges(0, 511)             | createClearRanges(512, 1023)
+        createPageRanges(0, 511, 1024, 1535) | createPageRanges(512, 1023, 1536, 2047) | createPageRanges(0, 511, 1024, 1535) | createClearRanges(512, 1023, 1536, 2047)
+    }
+
+    static def createPageRanges(long... offsets) {
+        def pageRanges = [] as List<PageRange>
+
+        if (CoreUtils.isNullOrEmpty(offsets)) {
+            return pageRanges
+        }
+
+        for (def i = 0; i < offsets.length / 2; i++) {
+            pageRanges.add(new PageRange().setStart(offsets[i * 2]).setEnd(offsets[i * 2 + 1]))
+        }
+
+        return pageRanges
+    }
+
+    static def createClearRanges(long... offsets) {
+        def clearRanges = [] as List<ClearRange>
+
+        if (CoreUtils.isNullOrEmpty(offsets)) {
+            return clearRanges
+        }
+
+        for (def i = 0; i < offsets.length / 2; i++) {
+            clearRanges.add(new ClearRange().setStart(offsets[i * 2]).setEnd(offsets[i * 2 + 1]))
+        }
+
+        return clearRanges
     }
 
     def "Get page ranges diff min"() {
@@ -867,7 +919,7 @@ class PageBlobAPITest extends APISpec {
         null     | null       | garbageEtag | null         | null           | null
         null     | null       | null        | receivedEtag | null           | null
         null     | null       | null        | null         | garbageLeaseID | null
-        null     | null       | null         | null        | null           | "\"notfoo\" = 'notbar'"
+        null     | null       | null        | null         | null           | "\"notfoo\" = 'notbar'"
     }
 
     def "Get page ranges diff error"() {
@@ -1178,13 +1230,13 @@ class PageBlobAPITest extends APISpec {
         bu2.copyIncrementalWithResponse(new PageBlobCopyIncrementalOptions(bc.getBlobUrl(), snapshot).setRequestConditions(mac), null, null).getStatusCode() == 202
 
         where:
-        modified | unmodified | match        | noneMatch    | tags
-        null     | null       | null         | null         | null
-        oldDate  | null       | null         | null         | null
-        null     | newDate    | null         | null         | null
-        null     | null       | receivedEtag | null         | null
-        null     | null       | null         | garbageEtag  | null
-        null     | null       | null         | null         | "\"foo\" = 'bar'"
+        modified | unmodified | match        | noneMatch   | tags
+        null     | null       | null         | null        | null
+        oldDate  | null       | null         | null        | null
+        null     | newDate    | null         | null        | null
+        null     | null       | receivedEtag | null        | null
+        null     | null       | null         | garbageEtag | null
+        null     | null       | null         | null        | "\"foo\" = 'bar'"
     }
 
     @Unroll
@@ -1204,18 +1256,18 @@ class PageBlobAPITest extends APISpec {
             .setTagsConditions(tags)
 
         when:
-        bu2.copyIncrementalWithResponse(new PageBlobCopyIncrementalOptions(bc.getBlobUrl(), snapshot).setRequestConditions(mac),null, null)
+        bu2.copyIncrementalWithResponse(new PageBlobCopyIncrementalOptions(bc.getBlobUrl(), snapshot).setRequestConditions(mac), null, null)
 
         then:
         thrown(BlobStorageException)
 
         where:
-        modified | unmodified | match       | noneMatch     | tags
-        newDate  | null       | null        | null          | null
-        null     | oldDate    | null        | null          | null
-        null     | null       | garbageEtag | null          | null
-        null     | null       | null        | receivedEtag  | null
-        null     | null       | null         | null         | "\"notfoo\" = 'notbar'"
+        modified | unmodified | match       | noneMatch    | tags
+        newDate  | null       | null        | null         | null
+        null     | oldDate    | null        | null         | null
+        null     | null       | garbageEtag | null         | null
+        null     | null       | null        | receivedEtag | null
+        null     | null       | null        | null         | "\"notfoo\" = 'notbar'"
     }
 
     def "Start incremental copy error"() {