Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

fix issue with none current page responses #30

Closed schaabs closed 7 years ago

msftclas commented 7 years ago

@schaabs, Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA. Thanks, Microsoft Pull Request Bot

codecov-io commented 7 years ago

Codecov Report

Merging #30 into master will decrease coverage by 0.1%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
- Coverage   88.33%   88.22%   -0.11%     
==========================================
  Files          10       10              
  Lines        1140     1138       -2     
==========================================
- Hits         1007     1004       -3     
- Misses        133      134       +1
Impacted Files Coverage Δ
msrest/paging.py 89.58% <100%> (ø) :arrow_up:
msrest/__init__.py 44.44% <0%> (-15.56%) :arrow_down:
msrest/version.py 100% <0%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 166413e...8aec749. Read the comment docs.

lmazuel commented 7 years ago

Hi @schaabs

LGTM, but current_page is not supposed to be None. Ever. Could you share how you end up with this issue? Might worth adding a unittest.

schaabs commented 7 years ago

@lmazuel,

Currently the azure key vault service returns null as a value when responding to a get on collections with no items. Our C# and Java clients are tolerant to this and simply don't iterate. However in python we raise a type error when we len(self.current_page) since current page is None. Ideally this will be fixed on the service side, but I'm proposing this fix to unblock python keyvault clients in the meantime. Let me know what you think.

Below is an example call from the keyvault python sdk tests which demonstrate this problem:

  request:
    body: null
    headers:
      Accept: [application/json]
      Accept-Encoding: ['gzip, deflate']
      Connection: [keep-alive]
      Content-Type: [application/json; charset=utf-8]
      User-Agent: [python/3.5.2 (Windows-10-10.0.15063-SP0) requests/2.17.3 msrest/0.4.8
          msrest_azure/0.4.8 keyvaultclient/0.3.4 Azure-SDK-For-Python]
      accept-language: [en-US]
      x-ms-client-request-id: [cfc77aee-4bb0-11e7-bfcc-5065f34efe31]
    method: GET
    uri: https://vault-397e1582.vault.azure.net/certificates?api-version=2016-10-01
  response:
    body: {string: '{"value":null,"nextLink":null}'}
    headers:
      Cache-Control: [no-cache]
      Content-Length: ['30']
      Content-Type: [application/json; charset=utf-8]
      Date: ['Wed, 07 Jun 2017 18:40:43 GMT']
      Expires: ['-1']
      Pragma: [no-cache]
      Server: [Microsoft-IIS/8.5]
      Strict-Transport-Security: [max-age=31536000;includeSubDomains]
      X-AspNet-Version: [4.0.30319]
      X-Content-Type-Options: [nosniff]
      X-Powered-By: [ASP.NET]
      x-ms-keyvault-region: [westus]
      x-ms-keyvault-service-version: [1.0.0.813]
      x-ms-request-id: [d905dc3c-adaa-45df-820f-572bebc1d8e6]
    status: {code: 200, message: OK}