Azure / azure-libraries-for-java

Azure Management Libraries for Java
https://docs.microsoft.com/en-us/java/azure/
MIT License
94 stars 98 forks source link

[BUG] Next page link missing while listing Storage accounts #1266

Closed jejner closed 4 years ago

jejner commented 4 years ago

Describe the bug While listing storage accounts via the list call next page link is missing in the sdk response and hence not able to fetch the remaining storage accounts. I guess the problem is here: https://github.com/Azure/azure-libraries-for-java/blob/v1.36.1/azure-mgmt-storage/src/main/java/com/microsoft/azure/management/storage/implementation/StorageAccountsInner.java is using PageImpl as the transformer pojo which has the missing @JsonProperty("nextLink") on nextPageLink attribute. Hence the next page link remains empty. It is not the same case with DisksInner where PageImpl1 is being used instead of PageImpl as the former class has the correct annotation present.

This issue looks similar to https://github.com/Azure/azure-libraries-for-java/issues/136.

To Reproduce Steps to reproduce the behavior: 1) List storage accounts call was made multiple times in a very short span of time.

Expected behavior NextPageLink should get filled with nextLink received from the rest api response.

Setup (please complete the following information):

yungezz commented 4 years ago

hi @xccc-msft could you pls help to have a look? thanks

xseeseesee commented 4 years ago

@jejner I'm not able to reproduce. Please share more info about your scenario. Thanks.

The test I tried to reproduce by listing storage accounts:

        ResourceGroup resourceGroup = azure.resourceGroups().define("RG_NAME")
                .withRegion(Region.US_WEST)
                .create();

        StorageAccount sa1 = azure.storageAccounts().define("SA_1")
                .withRegion(Region.US_WEST)
                .withExistingResourceGroup("RG_NAME")
                .create();

        StorageAccount sa2 = azure.storageAccounts().define("SA_2")
                .withRegion(Region.US_WEST)
                .withExistingResourceGroup("RG_NAME")
                .create();

        StorageAccount sa3 = azure.storageAccounts().define("SA_3")
                .withRegion(Region.US_WEST)
                .withExistingResourceGroup("RG_NAME")
                .create();

        PagedList<StorageAccount> list = azure.storageAccounts().list();
        for (StorageAccount sa : list) {
            System.out.println(sa.name() + " : " + sa.id());
        }
weidongxu-microsoft commented 4 years ago

@xccc-msft At least you need more than one page? (4 is not likely be the limit on the page)

jejner commented 4 years ago

@xccc-msft we are having more than 80 storage accounts spread across various regions & resource groups and the same azure account is being used in multiple dev environments. There were multiple calls for listing storage accounts from various envs in a very short span of time and we hit this issue of pagination. I was able to reproduce this by adding the listing storage accounts call in a for loop with more than 100 iterations.

xseeseesee commented 4 years ago

@jejner I tried to create 150 storage accounts with 5 regions/10 resource groups randomly. And then list all storage accounts. It works fine for me. Can you please capture the response when reproducing this issue? Also, do you have any scenario deleting storage accounts in some dev environments? It might possible if some deletion happened during the list operation so the list operation stopped.

jejner commented 4 years ago

@xccc-msft One time listing of storage accounts works fine, problem is when you fire multiple listing calls together(like more than 100 requests). With the 150 storage accounts you have created, can you try listing all storage accounts like 100 or 120 times via a loop? Then you will be able to see the issue. Right now i am using another Azure subscription which has 51 storage accounts Test code:

for (int i=1; i<=120; i++) {
            PagedList<StorageAccountInner> list = azure.storageAccounts().inner().list();
            System.out.println("Iteration Number: " + i);
            System.out.println("size: " + list.currentPage().items().size());
            System.out.println("isNextPagePresent: " + list.hasNextPage());
        }

and this is the response:

Iteration Number: 97
size: 51
isNextPagePresent: false
Iteration Number: 98
size: 51
isNextPagePresent: false
Iteration Number: 99
size: 48
isNextPagePresent: false
Iteration Number: 100
size: 48
isNextPagePresent: false
Iteration Number: 101
size: 48
isNextPagePresent: false

As for the deletion query, no storage accounts are being deleted from any of the environments.

Also when i am using the storage accounts list rest api, i am seeing the nextLink present in the response but when using the sdk this page link goes missing.

xseeseesee commented 4 years ago

@jejner I tried with same scenario as you suggested

        System.out.println("list by StorageAccountInner:");
        for (int i=1; i<=120; i++) {
            PagedList<StorageAccountInner> list = azure.storageAccounts().inner().list();
            System.out.println("Iteration Number: " + i);
            System.out.println("size: " + list.currentPage().items().size());
            System.out.println("isNextPagePresent: " + list.hasNextPage());
        }

        System.out.println("\n\nlist by StorageAccount:");
        for (int i=1; i<=120; i++) {
            PagedList<StorageAccount> list = azure.storageAccounts().list();
            System.out.println("Iteration Number: " + i);
            System.out.println("size: " + list.currentPage().items().size());
            System.out.println("isNextPagePresent: " + list.hasNextPage());
        }

And the responses for me are all the same

Iteration Number: 68
size: 151
isNextPagePresent: false
Iteration Number: 69
size: 151
isNextPagePresent: false
Iteration Number: 70
size: 151
isNextPagePresent: false
Iteration Number: 71
size: 151

Can you please try to capture the response and check if nextPageLink is missing in the response? Thanks.

jejner commented 4 years ago

@xccc-msft Can you try running the test code it multiple times or increase the number of iterations? It is happening for me consistently. Screenshots of response i am receiving while running the test code:

responseContentBodyHaving-nextLink deserializingResponseContentAndNextPageLinkIsNotFilled

I took these screenshots while debugging sdk code and specifically in the ServiceResponseBuilder class. As you can see nextLink is present in responseContent but nextPageLink is null when deserialised.

Also i will share below the rest api response called via postman which has the "nextLink" field present, so i am sure that results are getting divided between pages and is consistent with the test code result.

Screenshot 2020-08-28 at 11 52 50 AM

Problem is when this response is being transformed in the sdk layer. As i had mentioned in the bug description that "nextLink" is not being mapped to "nextPageLink", hence nextPageLink never gets filled.

I found this file: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/storage/mgmt-v2019_04_01/src/main/java/com/microsoft/azure/management/storage/v2019_04_01/implementation/StorageAccountsInner.java where the issue seems to be fixed as "PageImpl1" is being used as a transformer. There seems to be inconsistency between this file and the file mentioned in the bug description https://github.com/Azure/azure-libraries-for-java/issues/1266#issue-685169989 with regards to transformer implementation. Hope this info helps..

xseeseesee commented 4 years ago

@jejner Thanks for sharing. I think for my tests I always get only one page in the response so that I cannot reproduce. And for your case, there are more than one page in the response while the PageImpl is missing property of nextLink. After checking the PageImpl, it's a generated class by our code generator and swagger definitions. I just create a PR to fix this. Please feel free to review and share your comments. Thanks again for reporting this issue.