Remove chardet/charset-normalizer. Add fallback_charset_resolver ClientSession parameter.

[john-parton]

What do these changes do?

Continuing work from #7559

This replaces cchardet with chardetng and also paves a path towards deprecating character detection completely. It will issue a DeprecationWarning if the character detection method is invoked

Need updates.

Are there changes in behavior for the user?

Previously, encoding detection was done on every request without a content-type header. This was a bottleneck in some code I wrote. This changes it so that first utf-8 is tried, and then encoding detection happens aftewards.

Need updates.

Related issue number

Should close #7126

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[john-parton]

Note: When we decide on which approach to go with, I'll close the other two, and polish the remaining pull request. I haven't done docs or release notes quite as well as I would like.

[Dreamsorcerer]

I think the best approach would be to implement the new callable in master and backport to 3.9.
Then do a backport to 3.8, where we set the default callable to the existing chardet approaches, so behaviour should work the same in 3.8, while still allowing people to make the change that will be needed for 3.9.

[john-parton]

@Dreamsorcerer

I've updated this pull request with your suggestions, with a few tweaks

1. ClientSession now takes the argument default_encoding. I added this to the __init__ function and the corresponding private property to __slots__.
2. I chose default_encoding over fallback_encoding because the word default is used elsewhere and is a more familiar word
3. The valid types for default_encoding are None, str, Callable[[ClientRequest], str]. I chose only to pass the ClientRequest and not the body because in other places in the code where callables like this are passed around, that's usually how it's done. However, as I'm typing this I realize that a user will have to access _body directly, and won't be able to use read() because it's a coro. We can change the signature for this to include the body, or we can make it return Awaitable[str] if we want to enforce that the only parameter is ClientRequest, but also allow accessing of _body via a public method.

If this general approach looks good, I'll start updating the docs.

[Dreamsorcerer]

I chose default_encoding over fallback_encoding because the word default is used elsewhere and is a more familiar word

I feel like default makes it sound more like it should be a constant than a function, though I realise fallback isn't a lot better. Maybe something else like charset_func or something..?

The valid types for default_encoding are None, str, Callable[[ClientRequest], str].

I think this adds unnecessary complexity and performance overhead. If we just accept a callable only, it is trivial for someone to do lamba *args: "encoding", and likewise for the default value.

I chose only to pass the ClientRequest and not the body because in other places in the code where callables like this are passed around, that's usually how it's done.

The one thing that a charset guesser will need is the body (or part of it) and the text() method will already have read the body, so I think we should just pass it through and same any trouble.

[john-parton]

Charset guesser might also need the tld, as we already discussed. Removing str seems reasonable as well.

So I think Callable[[ClientResponse, bytes], str] and None would cover it?

[john-parton]

We could literally just make the function called encoding_guesser or charset_guesser.

[Dreamsorcerer]

Charset guesser might also need the tld, as we already discussed.

It's available in the Response, which I why I included it in the original examples.

So I think Callable[[ClientResponse, bytes], str] and None would cover it?

We don't need None, just default to lambda: r, b: "utf-8".

[Dreamsorcerer]

We could literally just make the function called encoding_guesser or charset_guesser.

Yeah, I guess that's fine, just might be a little confusing given that we default to a function which doesn't do any guessing.

[john-parton]

Okay, I changed the variable/callback to 'fallback_encoding', I change the type signature to just be Optional[Callable], and I updated code in appropriate places.

As far as code snippets are concerned for instructing a user on how to operate the character set detection once the feature is removed, here are two options

# chardetng_py: fast charset detection written in rust
from chardetng_py import detect

def fallback_encoding(response, body):
    tld = response.url.host.split(".")[-1]
    return detect(body, tld=tld)

# cChardet, chardet, charset_normalizer
from charset_normalizer import chardet

def fallback_encoding(response, body):
    return chardet(body)["encoding"] or "utf-8"

async with ClientSession(fallback_encoding=fallback_encoding) as client:
    ....

[Dreamsorcerer]

I think things are getting mixed up a little.

Can we start with a PR that removes chardet and supports the new parameter? This will go into master. Once that's in we can look at backports and including chardetng in the 3.8 backport.

[john-parton]

No problem.

[john-parton]

@Dreamsorcerer So this removes the character set detection now and introduces the fallback_encoding callback we talked about.

Do you want me to update the ClientSession documentation and the relevant text() method documentation to refer to that?

[Dreamsorcerer]

Do you want me to update the ClientSession documentation and the relevant text() method documentation to refer to that?

Yep, we'll want that included too with examples of correctly using it for chardetng and charset-normalizer.

[john-parton]

@Dreamsorcerer I did my best. I'm not great at writing documentation. I tried to make some of the finer points clear, and I documented that text() will raise UnicodeDecodeError if not configured correctly, and I added a similar note in the "advanced usage" section that I added, so hopefully that will help with google-abilty-of-error.

[patchback]

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 6755796 on top of patchback/backports/3.9/675579699422680607108a7dd68c85ec5284220c/pr-7561

Backporting merged PR #7561 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.9/675579699422680607108a7dd68c85ec5284220c/pr-7561 upstream/3.9

4. Now, cherry-pick PR Remove chardet/charset-normalizer. Add fallback_charset_resolver ClientSession parameter. #7561 contents into that branch:

   $ git cherry-pick -x 675579699422680607108a7dd68c85ec5284220c

   If it'll yell at you with something like fatal: Commit 675579699422680607108a7dd68c85ec5284220c is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 675579699422680607108a7dd68c85ec5284220c

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Remove chardet/charset-normalizer. Add fallback_charset_resolver ClientSession parameter. #7561 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/675579699422680607108a7dd68c85ec5284220c/pr-7561

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to 3.8: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 6755796 on top of patchback/backports/3.8/675579699422680607108a7dd68c85ec5284220c/pr-7561

Backporting merged PR #7561 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.8/675579699422680607108a7dd68c85ec5284220c/pr-7561 upstream/3.8

4. Now, cherry-pick PR Remove chardet/charset-normalizer. Add fallback_charset_resolver ClientSession parameter. #7561 contents into that branch:

   $ git cherry-pick -x 675579699422680607108a7dd68c85ec5284220c

   If it'll yell at you with something like fatal: Commit 675579699422680607108a7dd68c85ec5284220c is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 675579699422680607108a7dd68c85ec5284220c

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Remove chardet/charset-normalizer. Add fallback_charset_resolver ClientSession parameter. #7561 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.8/675579699422680607108a7dd68c85ec5284220c/pr-7561

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[Dreamsorcerer]

@john-parton If you could handle the backports, that'd be great. We'll need to:

• Follow the instructions above for 3.9 backport. This should be a straight backport.
• Follow the instructions above for 3.8 backport.
  • Modify the 3.8 backport to change the default function to emulate the current chardet behaviour (preferably without chardetng).
  • Add a DeprecationWarning in the default function when it is called (or maybe when it returns a non "utf-8" value?).
• Once they are both merged, create a new PR on 3.8 branch that replaces cchardet with chardetng.

[webknjaz]

Does this actually work? The correct syntax AFAIK doesn't include ().

[webknjaz]

And so it doesn't: https://aiohttp--7561.org.readthedocs.build/en/7561/client_advanced.html#character-set-detection.

cc @Dreamsorcerer

[Dreamsorcerer]

Ah, sorry, I actually copied from the section above, which makes the same mistake with .close(). I wonder why Sphinx doesn't produce a warning...

[webknjaz]

Dunno. Maybe because of this ignore https://github.com/aio-libs/aiohttp/blob/6755796/docs/conf.py#L384 or some Sphinx but, or a corner case. Somebody needs to go through all those ignores and fix them.

[webknjaz]

@Dreamsorcerer so it looks like we don't run the normal docs build @ GHA and spellcheck that we do run doesn't emit those warnings: https://github.com/aio-libs/aiohttp/actions/runs/6018473896/job/16326723444#step:13:18.

OTOH, RTD does have those warnings in the log but isn't set up to turn them into errors: https://readthedocs.org/projects/aiohttp/builds/21761448/.

I think the CI was set up to fail on warnings at some point. Maybe that got removed, or I'm just confusing the repos…

[Dreamsorcerer]

Well, other warnings seem to fail CI in the docs spelling step (I'm not aware of another part that produces warnings):
https://github.com/aio-libs/aiohttp/actions/runs/6017616744/job/16324167311#step:13:86

[john-parton]

@john-parton If you could handle the backports, that'd be great. We'll need to:

I don't have the time to dedicate this week.

[Dreamsorcerer]

3.9 backport done: #7586
If you have some time to handle the 3.8 backport and finish #7561 (comment) soon, that'd be great. Hoping to get a new release out shortly.

[john-parton]

I'll take a crack at it later today.

[john-parton]

@Dreamsorcerer PR opened here: #7589