[core] add provisional azure.core.rest #19502

iscai-msft · 2021-06-25T20:22:39Z

No description provided.

…into get_testserver_working * 'master' of https://github.com/Azure/azure-sdk-for-python: (55 commits) [AppConfig] mypy fixes (#18858) [translation] renames (#19285) [Key Vault] Update metadata for release (#19286) [AppConfig] enable samples (#18857) KeyVaultBackupOperation -> KeyVaultBackupResult (#19284) renaming (#19280) [Key Vault] Custom polling method for admin backup client (#19204) [textanalytics] add ARM template + run samples in CI (#19270) removed example from description (#18781) [Key Vault] Update READMEs with RBAC information (#19275) Sync eng/common directory with azure-sdk-tools for PR 1688 (#19272) update all ubuntu vmImage to 20.04 (#19227) add base class for feedback (#19265) Enable tests for integration samples (#18531) docs: fix a few simple typos (#19127) Increment package version after release of azure-eventgrid (#19197) Increment package version after release of azure-monitor-query (#19208) earliest versions of communication identity tests are set up improperly. skip 1.0.0 in regression testing (#19258) Run mypy in azure-keyvault-administration CI (#19246) [text analytics] change return type of analyze actions to list of list (#18994) ...

…into get_testserver_working * 'main' of https://github.com/Azure/azure-sdk-for-python: (45 commits) ignore coretestserver readme (#19436) Add Ubuntu 20 to local dns bypass template (#19432) Sync eng/common directory with azure-sdk-tools for PR 1729 (#19415) Async/BearerTokenCredentialPolicy consistently calls on_exception (#19195) [EventHubs] Fix bug in sending stress test code and update default stress test settings (#19429) [EventHubs] Get IoT Hub Name from Redirect Address in sample (#19314) [textanalytics] regen on v3.1 (#19193) Adapt EG to arm template (#19262) [Key Vault] Extend pipeline test timeout (#19404) Update platform matrix to ubuntu 20 (#19296) [AppConfig] Add lock to SyncTokenPolicy (#19395) Regenerate monitor code (#19375) Increment version for keyvault releases (#19402) Aggregation should be a list (#19381) [azure-mgmt-monitor] skip test to unblock ci (#19390) Cloud event should parse smaller ms precisions (#19259) Update release date (#19399) [Communication]: use x-ms-date for hmac (#19396) [Key Vault] Performance tests for certificates, keys, and secrets (#19002) Deprecate azure-monitor (#19384) ...

xiangyan99 · 2021-06-28T22:01:09Z

sdk/core/azure-core/azure/core/pipeline/_tools_async.py

+    except ImportError:
+        pass
+    try:
+        from .transport import AsyncioRequestsTransportResponse


Do we need to import all of them?

Not only needed in except ImportError?

i can add a check for each of these to only try to import if response_type is still None. But basically I put them each into their own try except import error block bc, for example, if you have a trio transport and no aiohttp isntalled, importing aiohttp will cause an error for you, so I wanted to accomodate all of the different combos

ok, I now stop importing once I get a reseponse type. Thanks for the suggestion @xiangyan99 !

sdk/core/azure-core/azure/core/rest/_aiohttp.py

sdk/core/azure-core/azure/core/rest/_helpers.py

johanste · 2021-06-28T22:14:24Z

sdk/core/azure-core/azure/core/_pipeline_client.py

+        if rest_request:
+            response = _to_rest_response(response)
+            if not kwargs.get("stream", False):
+                response.read()


Will we leak a stream if read fails?

good point, I'm going to add a try except when I read the response, and close the response before throwing the error. thanks!

sdk/core/azure-core/azure/core/_pipeline_client_async.py

sdk/core/azure-core/azure/core/exceptions.py

xiangyan99 · 2021-06-28T23:07:00Z

sdk/core/azure-core/azure/core/rest/_aiohttp.py

+        ctype = self.headers.get(aiohttp.hdrs.CONTENT_TYPE, "").lower()
+        mimetype = aiohttp.helpers.parse_mimetype(ctype)
+
+        encoding = mimetype.parameters.get("charset")


Given we set encoding here, why we need L80?

I'm following the strucutre for the code in pipeline transport. But basically, it seems that if the chardet detecting fails, encoding still might be none, so we fall back to "utf-8-sig"

I mean L84 will override L80, right?

I cleaned this up a bit, lmk your thoughts, thanks!

xiangyan99 · 2021-06-28T23:08:53Z

sdk/core/azure-core/azure/core/rest/_rest.py

+        """
+        raise NotImplementedError()
+
+    def iter_text(self):


Do you override iter_text for aiohttp?

If not, how can we make sure self.encoding works fine?

I ended up moving parts of the encoding into the base class for encoding.
Basically, 1. if we have an application/json, following RFC 7159 and 7483, I default this to "utf-8". 2. if the response's content has been read in, I'll add an additional step of using chardet.detect to get the response's content. With this, the self.encoding should cover more corner cases with iter_text

Is it possible iter_text is called w/o .read()?

In this case, content is empty?

yeah definitely. Basically for iter_bytes, iter_text, and iter_lines, if content exists, we'll yield the content to users. Otherwise, we'll call stream download generator and get the bytes from there.

This is the same behavior as httpx

You calls iter_bytes or content?

It seems to me iter_bytes does not generate content?

here's kind of the scenarios where you would use one vs. the other

I want the whole body on the response, and I want to access it all at once: Here you would call .content, similar to .body() on the old responses. You may need to call .read() first, to read in the response's bytes to content before

I want to iterate over the body: here, i call one of the iter_ functions. This doesn't read the body into the response, but outputs it as an iterator. As more of an implementation detail: if the response's bytes are already read into content, we will iterate over the content in all of the iter_ functions except iter_raw (consistent with httpx), and output them in chunks <= the block size we get from the transport

I think we are on same page. So if the content is not read into the body, iter_text will not be able to detect the encoding from content, right?

It still will be able. to do all of the checks, except the final fallback I added, which is the chardet.detect of the content

…rrors

…as well

xiangyan99 · 2021-06-29T00:43:04Z

sdk/core/azure-core/azure/core/rest/_rest.py

+        return self.content.decode(encoding)
+
+    @property
+    def num_bytes_downloaded(self):


Please hide it. There is no reliable way to return the bytes.

All I do in num_bytes_downloaded is I get. the length of the chunk before I send it to users, I feel like that's p reliable, why are you thinking it's not reliable? sorry if I'm missing something

in compression scenario. Which num is correct? Before decompression or after?

you're right that seems fair, i can remove for now

annatisch · 2021-06-29T00:43:10Z

sdk/core/azure-core/azure/core/pipeline/_tools.py

+        data=pipeline_transport_request.data
+    )
+
+def to_rest_response(pipeline_transport_response):


What will this method do if users have created their client with a custom transport?
Does it matter here?

if they create their client with a custom transport, since i don't have a corresponding rest transport response to change it to, I change it to the default azure.core.rest.HttpResponse or azure.core.rest.AsyncHttpResponse

annatisch

LGTM - can't see anything that would break existing pipelines.
Haven't dug too deep into the new implementation of the new rest implementation, but API surface looks good and well documented as provisional :)

iscai-msft · 2021-06-29T01:33:04Z

/azp run python - core - ci

azure-pipelines · 2021-06-29T01:33:22Z

Azure Pipelines successfully started running 1 pipeline(s).

iscai-msft · 2021-06-29T01:45:22Z

/azp run python - communication - ci

azure-pipelines · 2021-06-29T01:45:40Z

Azure Pipelines successfully started running 1 pipeline(s).

iscai-msft · 2021-06-29T15:14:41Z

/azp run python - identity - ci

azure-pipelines · 2021-06-29T15:14:58Z

Azure Pipelines successfully started running 1 pipeline(s).

lmazuel

Did a short fast review, LGTM :)

iscai-msft added 30 commits June 8, 2021 12:55

tests passing locally

6dff5d5

move pytests.init to root

58c184a

remove set formatting from windows run

c7663fe

try to fix windows testserver start

bec31ed

uncomment testserver termination

405d5e8

add devops scripts to start testserver

f826377

scriptSource -> scriptPath

6ceb795

set env vars

0f10e5e

add pwsh testserver

c35fdc2

return result

f8fd247

try retuning exit code

c4fd6d2

remove ci work, make pytest fixture module level

b322e28

tests working without pytest.ini

449f42a

only have testserver fixture in conftest

a021de2

switch to package scope

7a68dd3

unite testserver setting

df42f15

switch to environment variables

da62f4b

see what happens if we don't kill testserver

e06ebb4

cycle through ports

04ddbd0

remove scripts

e860e8a

allow 2.7 compatibility

2f39ad2

wait longer for pypy

f503b2c

increase sleep to 2 for pypy

8ccce34

move core testserver into tests

30c6e39

switch to urllib requesting to see if port open

20e593a

ignore coretestserver readme

1483f8c

initial commit for rest layer

aabe409

add rest transport responses

cf637ef

xiangyan99 reviewed Jun 28, 2021

View reviewed changes

stop importing resposne types once we get one

7fd947f

xiangyan99 reviewed Jun 28, 2021

View reviewed changes

sdk/core/azure-core/azure/core/rest/_aiohttp.py Outdated Show resolved Hide resolved

set decompress property for aiohttp in to_rest_response

931aba3

xiangyan99 reviewed Jun 28, 2021

View reviewed changes

sdk/core/azure-core/azure/core/rest/_helpers.py Show resolved Hide resolved

johanste reviewed Jun 28, 2021

View reviewed changes

sdk/core/azure-core/azure/core/_pipeline_client_async.py Outdated Show resolved Hide resolved

johanste reviewed Jun 28, 2021

View reviewed changes

sdk/core/azure-core/azure/core/exceptions.py Show resolved Hide resolved

xiangyan99 reviewed Jun 28, 2021

View reviewed changes

iscai-msft added 2 commits June 28, 2021 19:16

remove special content from aiohttp and add reuest info to new rest e…

27f4af9

…rrors

default application/json to utf-8, and use chardet to detect endoing …

9d4013b

…as well

xiangyan99 reviewed Jun 29, 2021

View reviewed changes

annatisch reviewed Jun 29, 2021

View reviewed changes

iscai-msft added 2 commits June 28, 2021 20:44

read body in a try except so we can close if it fails

59f0015

remove num_bytes_downloaded

a575549

iscai-msft requested review from xiangyan99, johanste and annatisch June 29, 2021 01:07

annatisch approved these changes Jun 29, 2021

View reviewed changes

lmazuel approved these changes Jun 29, 2021

View reviewed changes

iscai-msft merged commit 662dedf into main Jun 29, 2021

iscai-msft deleted the rest_layer branch June 29, 2021 18:29

rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Jul 16, 2021

[core] add provisional azure.core.rest (Azure#19502)

7bf8c77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] add provisional azure.core.rest #19502

[core] add provisional azure.core.rest #19502

iscai-msft commented Jun 25, 2021

xiangyan99 Jun 28, 2021

iscai-msft Jun 28, 2021

iscai-msft Jun 28, 2021

johanste Jun 28, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 28, 2021

iscai-msft Jun 28, 2021

xiangyan99 Jun 28, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 28, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 29, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 29, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 29, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 29, 2021

iscai-msft Jun 29, 2021

xiangyan99 Jun 29, 2021 •

edited

Loading

iscai-msft Jun 29, 2021

annatisch Jun 29, 2021

iscai-msft Jun 29, 2021

annatisch left a comment

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

lmazuel left a comment

[core] add provisional azure.core.rest #19502

[core] add provisional azure.core.rest #19502

Conversation

iscai-msft commented Jun 25, 2021

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

xiangyan99 Jun 29, 2021 • 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

annatisch left a comment

Choose a reason for hiding this comment

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

iscai-msft commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

lmazuel left a comment

Choose a reason for hiding this comment

xiangyan99 Jun 29, 2021 •

edited

Loading