New exception when redirect url is invalid

[setla]

trafficstars

What do these changes do?

Added new exception type to separate cases with invalid redirect url during request. Previously ValueError or InvalidURL was raised and screening out was hard (valid url that redirects to invalid one raised the same error as invalid url).

Slightly improved test coverage.

Are there changes in behavior for the user?

In case of invalid redirect url during request - new InvalidRedirectUrl will be raised instead of ValueError or InvalidURL, it derives from InvalidURL so it's backward compatibile

Related issue number

https://github.com/aio-libs/aiohttp/issues/2630

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [x] Documentation reflects the changes
• [x] 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.
• [x] 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."

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0ec65c0) 97.51% compared to head (80b5479) 97.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6722      +/-   ##
==========================================
+ Coverage   97.51%   97.52%   +0.01%     
==========================================
  Files         107      107              
  Lines       32733    32802      +69     
  Branches     3824     3840      +16     
==========================================
+ Hits        31920    31991      +71     
+ Misses        612      610       -2     
  Partials      201      201              

Flag	Coverage Δ
CI-GHA	97.44% <100.00%> (+0.01%)	:arrow_up:
OS-Linux	97.12% <100.00%> (+0.01%)	:arrow_up:
OS-Windows	95.62% <100.00%> (+<0.01%)	:arrow_up:
OS-macOS	96.75% <100.00%> (-0.18%)	:arrow_down:
Py-3.10.11	95.54% <100.00%> (+<0.01%)	:arrow_up:
Py-3.10.13	96.92% <100.00%> (+0.01%)	:arrow_up:
Py-3.11.7	?
Py-3.11.8	96.57% <100.00%> (?)
Py-3.12.1	?
Py-3.12.2	96.69% <100.00%> (?)
Py-3.8.10	95.51% <100.00%> (+<0.01%)	:arrow_up:
Py-3.8.18	96.86% <100.00%> (+0.01%)	:arrow_up:
Py-3.9.13	95.51% <100.00%> (+<0.01%)	:arrow_up:
Py-3.9.18	96.89% <100.00%> (+0.01%)	:arrow_up:
Py-pypy7.3.15	96.43% <100.00%> (+0.01%)	:arrow_up:
VM-macos	96.75% <100.00%> (-0.18%)	:arrow_down:
VM-ubuntu	97.12% <100.00%> (+0.01%)	:arrow_up:
VM-windows	95.62% <100.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[setla]

@Dreamsorcerer can I get some clue if (and when if so) it's going to be merged?

[webknjaz]

@setla looks like a few files diverged since your last update — could you fix the merge conflicts?

@Dreamsorcerer has this slipped off of your review queue?

[Dreamsorcerer]

@Dreamsorcerer has this slipped off of your review queue?

Seems so. I probably still have a tab open to come back to it somewhere.. This looks like it's basically ready.

[setla]

@Dreamsorcerer @webknjaz please let me know if there is anything else to change and resolve conversations if changes cover it

[webknjaz]

@setla another round of reviews complete. I remembered some old discussions and made some structural requests based on them. Plus this would make the different cases even more easier to distinguish.

[wumpus]

Thanks for keeping this issue alive. I stopped using aiohttp's redirect code because I had no idea what the correct behavior should be, thus I did not know what a patch should do.

[setla]

@Dreamsorcerer @webknjaz is it expected that windows tests fail? https://github.com/setla/aiohttp/actions/runs/7765565978/job/21180923429?pr=1 - it shouldn't be related to changes in this PR.

[Dreamsorcerer]

@Dreamsorcerer @webknjaz is it expected that windows tests fail? https://github.com/setla/aiohttp/actions/runs/7765565978/job/21180923429?pr=1 - it shouldn't be related to changes in this PR.

You only need to worry if the required statuses fail (in this case, check).

[setla]

@webknjaz please take a look, i really want to merge this PR :)

[webknjaz]

@setla I also really want to merge it. Though, I insist on the exception hierarchy to be corrected first. Non-HTTP URLs are not necessarily invalid, just different. The clients should use ClientError if they want to have a catch-all. I think @Dreamsorcerer mentioned that the users expect this anyway.

[setla]

@setla I also really want to merge it. Though, I insist on the exception hierarchy to be corrected first. Non-HTTP URLs are not necessarily invalid, just different. The clients should use ClientError if they want to have a catch-all. I think @Dreamsorcerer mentioned that the users expect this anyway.

@webknjaz so should I replace InvalidUrl by ClientError asNonHttpUrlClientError parent class?

[webknjaz]

so should I replace InvalidUrl by ClientError asNonHttpUrlClientError parent class?

Yes, make it follow my last graph, please. Sam said that some people may expect it to be one way and the others would expect it the other way, so I'm going to insist on my way since it's impossible to satisfy all parties in such cases. The important thing is that in both cases, it's a client error that remains a catchable base.

[webknjaz]

@setla I was hoping that applying small changes via GH UI would be enough to get this going but it seems like it needs some editing still. Plz pull the new commits from here before attempting to fix the tests.

[setla]

@webknjaz fixed, it seems like everything is addressed now, please let me know if there is anything else to do

[patchback[bot]]

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

❌ Failed to cleanly apply fb465e155b872f01489173d11e35f02ccbf3a940 on top of patchback/backports/3.10/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722

Backporting merged PR #6722 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.10/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722 upstream/3.10
   

4. Now, cherry-pick PR #6722 contents into that branch:

   $ git cherry-pick -x fb465e155b872f01489173d11e35f02ccbf3a940
   

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

   $ git cherry-pick -m1 -x fb465e155b872f01489173d11e35f02ccbf3a940
   

5. At this point, you'll probably encounter some merge conflicts. You must resolve them in to preserve the patch from PR #6722 as close to the original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.10/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722
   

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[bot]]

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

❌ Failed to cleanly apply fb465e155b872f01489173d11e35f02ccbf3a940 on top of patchback/backports/3.9/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722

Backporting merged PR #6722 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/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722 upstream/3.9
   

4. Now, cherry-pick PR #6722 contents into that branch:

   $ git cherry-pick -x fb465e155b872f01489173d11e35f02ccbf3a940
   

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

   $ git cherry-pick -m1 -x fb465e155b872f01489173d11e35f02ccbf3a940
   

5. At this point, you'll probably encounter some merge conflicts. You must resolve them in to preserve the patch from PR #6722 as close to the original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/fb465e155b872f01489173d11e35f02ccbf3a940/pr-6722
   

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.

[webknjaz]

@setla to get this released, backport PRs should be submitted per instructions above.

[setla]

@webknjaz right, should I create 2 PRs (3.9 and 3.10)? or just one (3.9)? 3.9 is already present https://github.com/aio-libs/aiohttp/pull/8154

[webknjaz]

Two — the branches are separate. The backports are to made in reversed order. Besides, I know that 3.10 is definitely necessary, but not sure about 3.9 — I'll wait to see if anybody objects since bits of this might be perceived as breaking/new behaviors and not just a bugfix. Though I'll be able to merge into 3.10 right away.

[Dreamsorcerer]

I'll wait to see if anybody objects since bits of this might be perceived as breaking/new behaviors and not just a bugfix.

I'm not sure it's important enough to bother going into 3.9. I think we have enough improvements in 3.10 that we should be planning a release in a couple of months anyway.

[webknjaz]

I'm not sure it's important enough to bother going into 3.9. I think we have enough improvements in 3.10 that we should be planning a release in a couple of months anyway.

I want to make a 3.9 release as soon as https://github.com/aio-libs/aiohttp/pull/8089 is in. So if there's anything that could piggyback with that for free, I'd rather include it for as long as it's eligible. OTOH, the new exceptions are a new API, so per SemVer it should go into a minor release, not patch. I lost sight of it, but I just checked that the change notes are of the feature type, which makes more sense. I suppose, a 3.9 backport isn't needed indeed.

[setla]

@webknjaz okay - I opened PR to 3.10 https://github.com/aio-libs/aiohttp/pull/8158

[webknjaz]

@setla thank you! This improvement is what I've been wanting to get done for years and now it's mere months away.. Watch for new releases of the 3.10 series coming up this year!

[setla]

thanks :)

[bdraco]

It looks like this introduced a regression with websocket urls

2024-02-16 09:45:14.978 ERROR (MainThread) [pyunifiprotect.websocket] Websocket disconnect error: %s
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/pyunifiprotect/websocket.py", line 95, in _websocket_loop
    self._ws_connection = await session.ws_connect(
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/aiohttp/client.py", line 884, in _ws_connect
    resp = await self.request(
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/aiohttp/client.py", line 472, in _request
    raise NonHttpUrlClientError(url)
aiohttp.client_exceptions.NonHttpUrlClientError: wss://192.168.209.164/proxy/protect/ws/updates?lastUpdateId=80f54055-84de-4e15-95d7-80cac46aafbd

[webknjaz]

@bdraco does this mean there's no test coverage for this case? Would you look into it? What's the expected behavior? What's supposed to handle this?