[core] remove chardet dep from azure.core.rest #19962

iscai-msft · 2021-07-27T16:41:56Z

No description provided.

lmazuel · 2021-07-27T17:23:05Z

nope :p

… httpx

xiangyan99 · 2021-07-27T18:53:15Z

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

-            try:
-                encoding = chardet.detect(response.content)["encoding"]
-            except ResponseNotReadError:
-                pass
    if encoding is None or not lookup_encoding(encoding):
        return None


We want to default to "utf-8"("utf-8-sig"), right?

After talking with @lmazuel, we don't default the value of self.encoding to utf-8-sig. We do that when we're deserializing for the text property, which I've added code for in def decode_to_text

iscai-msft · 2021-07-27T18:56:39Z

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

+    if encoding:
+        return content.decode(encoding)
+    # 1. We try utf-8-sig
+    decoder = codecs.getincrementaldecoder("utf-8-sig")(errors="strict")


followed httpx's lead, and included trying to deserialize with windows 1252 @lmazuel . Lmk if we want to remove this

https://github.com/encode/httpx/blob/master/httpx/_models.py#L1317

If utf-8-sig didn't work, utf-8 won't as well (so no need to do it later), utf-8-sig is a super-set of utf-8.

I wouldn't try cp1252 yet, httpx does it for HTML pages in old MS servers (we don't do that). Let's not fix what is not broken :)

ok sounds good, will simplify my code too. thanks!

xiangyan99 · 2021-07-30T16:32:47Z

sdk/core/azure-core/CHANGELOG.md

@@ -8,6 +8,8 @@

 ### Key Bugs Fixed

+- Removed `chardet` dependency in `azure.core.rest`.


Do we want to clarify this is a breaking change?

I'm not sure, @annatisch do you think we should clarify it's breaking, because this is in a provisional package? Worried we're being scary for non-azure.core.rest users for nothing

@xiangyan99 talked to @lmazuel and he recommended adding a section like "Breaking changes in provisional package", so I added that to the changelog and listed this fix under there. Thanks!

xiangyan99 · 2021-07-30T16:33:53Z

sdk/core/azure-core/tests/testserver_tests/test_rest_http_response.py

@@ -168,7 +168,7 @@ def test_json(send_request):
        request=HttpRequest("GET", "/basic/json"),
    )
    assert response.json() == {"greeting": "hello", "recipient": "world"}
-    assert response.encoding == 'utf-8-sig'  # for requests, we use utf-8-sig instead of utf-8 bc of requests behavior


Do we still need these three tests? Maybe one is good enought?

Not quite sure which tests you mean by the three tests, but I feel like more tests is better, so might as well not get rid of them

iscai-msft · 2021-08-02T17:46:01Z

/azp run python - core - ci

azure-pipelines · 2021-08-02T17:46:20Z

Azure Pipelines successfully started running 1 pipeline(s).

xiangyan99

The change looks good. Please confirm the changelog.

check-enforcer · 2021-08-02T19:26:33Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run python - [service] - ci

…into add_chardet_dep * 'main' of https://github.com/Azure/azure-sdk-for-python: (35 commits) `azure-applicationinsights` Hyperlinks need sphinx style formatting (Azure#19993) update version and changelog (Azure#20034) Rename AZURE_POD_IDENTITY_TOKEN_URL -> AZURE_POD_IDENTITY_AUTHORITY_HOST (Azure#19867) add sample for raw_response_hook (Azure#19985) cut hard dependency on requests (Azure#19930) Update ci.yml (Azure#20028) Update ci.yml (Azure#20026) Update PythonSdkLiveTest.yml (Azure#19999) [AutoRelease] t2-recoveryservicessiterecovery-2021-07-28-40357 (Azure#19970) [AutoRelease] t2-storagecache-2021-07-29-63871 (Azure#19987) Minor changes in docs (Azure#20016) Add Logs Batch query results (Azure#20013) Rename query (Azure#19952) d218371 (Azure#20009) Update CHANGELOG.md (Azure#20006) [Batch] Data plane SDK v11.0.0 (Azure#19889) [AutoRelease] t2-batch-2021-07-30-27488 (Azure#19996) add ga id to index (Azure#19995) Edit pass on Monitor Query README file (Azure#19979) Move stress testing scripts to eng/common (Azure#19994) ...

lmazuel · 2021-08-02T23:34:20Z

sdk/core/azure-core/CHANGELOG.md

-### Breaking Changes
+### Breaking Changes in the Provisional `azure.core.rest` package
+
+- Removed `chardet` dependency in `azure.core.rest`. Breaking because we removed the step that used `chardet` to inspect the response encoding


I would phrase it as impact on customer, not internal details:

azure.core.rest won't try to guess the charset anymore if it was impossible to extract it from HttpResponse analysis

Sounds good!

add chardet dep to setup.py

a13b248

iscai-msft requested review from lmazuel and xiangyan99 as code owners July 27, 2021 16:41

ghost added the Azure.Core label Jul 27, 2021

update changelog

2c9d57b

iscai-msft requested a review from annatisch July 27, 2021 17:00

lmazuel closed this Jul 27, 2021

remove chardet dependency, and improve text deserialization following…

9de3b62

… httpx

iscai-msft reopened this Jul 27, 2021

iscai-msft changed the title ~~[core] add chardet dep to setup.py~~ [core] remove chardet dep from azure.core.rest Jul 27, 2021

xiangyan99 reviewed Jul 27, 2021

View reviewed changes

iscai-msft commented Jul 27, 2021

View reviewed changes

remove cp1252 decoding

2be52d4

xiangyan99 reviewed Jul 30, 2021

View reviewed changes

update encoding docstring

916aafc

xiangyan99 approved these changes Aug 2, 2021

View reviewed changes

change changelog to breaking change in provisional

a7a1a85

lmazuel reviewed Aug 2, 2021

View reviewed changes

lmazuel approved these changes Aug 2, 2021

View reviewed changes

Update CHANGELOG.md

be24aa7

iscai-msft merged commit 6b9ac65 into Azure:main Aug 3, 2021

iscai-msft deleted the add_chardet_dep branch August 3, 2021 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] remove chardet dep from azure.core.rest #19962

[core] remove chardet dep from azure.core.rest #19962

iscai-msft commented Jul 27, 2021

lmazuel commented Jul 27, 2021

xiangyan99 Jul 27, 2021

iscai-msft Jul 27, 2021

iscai-msft Jul 27, 2021

lmazuel Jul 27, 2021

iscai-msft Jul 27, 2021

xiangyan99 Jul 30, 2021

iscai-msft Aug 2, 2021

iscai-msft Aug 2, 2021

xiangyan99 Jul 30, 2021

iscai-msft Aug 2, 2021

iscai-msft commented Aug 2, 2021

azure-pipelines bot commented Aug 2, 2021

xiangyan99 left a comment

check-enforcer bot commented Aug 2, 2021

lmazuel Aug 2, 2021

iscai-msft Aug 3, 2021

		@@ -8,6 +8,8 @@

		### Key Bugs Fixed

		- Removed `chardet` dependency in `azure.core.rest`.

[core] remove chardet dep from azure.core.rest #19962

[core] remove chardet dep from azure.core.rest #19962

Conversation

iscai-msft commented Jul 27, 2021

lmazuel commented Jul 27, 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

iscai-msft commented Aug 2, 2021

azure-pipelines bot commented Aug 2, 2021

xiangyan99 left a comment

Choose a reason for hiding this comment

check-enforcer bot commented Aug 2, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Choose a reason for hiding this comment

Choose a reason for hiding this comment