Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement granular URL error hierarchy in the HTTP client #6722
Implement granular URL error hierarchy in the HTTP client #6722
Changes from all commits
b0474ab
8c132cc
8217849
56f2408
416b1d6
3782f88
c6a32db
a6babca
2819150
3308058
7f37635
4f7c7b2
b8ff8af
05ffdb0
11f20e9
fe39f22
70921c5
aa7a63c
ff3100a
23d2e9f
b0a68cc
1612c92
a39e190
6a4cf9f
6357cf2
449cb33
7236ad1
5cdbab7
c0334c4
6011ced
9e5de99
d94cdff
8e824fd
9c1c159
ebd3c05
f96b76a
2fa0fa1
090fe74
c10075d
05aa519
ed13789
9733022
67ab859
d10d3d3
37871a0
1379128
c27bca3
9598332
537bb82
9c859a4
a910301
47a0043
0c3a1a3
56b4781
f815b94
d1e26d5
91c14a1
5f17628
98cd194
5af3f74
89f9757
86645df
d8e1d26
7a87251
cb00d40
05038d8
eda791d
c5e5b9e
2022cde
a04e61e
7c9194e
80b5479
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bdraco @setla would augmenting this make it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will analyze how its being used in the case I posted above when I get back home next week and provide additional detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a possible future improvement, I'd rename the constant to something like
SUPPORTED_URI_SCHEMAS
. Especially, if it'll stop doing listing http only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz @bdraco Mentioned exception is related because it's raised here https://github.com/aio-libs/aiohttp/pull/6722/files#diff-f334e752b4894ef951105572ab8b195aeb8db90eb6e48b1dfbd9a01da4c854f5R420 but it's also used in redirects check https://github.com/aio-libs/aiohttp/pull/6722/files#diff-f334e752b4894ef951105572ab8b195aeb8db90eb6e48b1dfbd9a01da4c854f5R647. If we don't want regression, probably first mentioned place should be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have two constants where one lists ws/wss and the check asserts against both in the first place but not the second one. Are websockets supposed to be disallowed in redirects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
ws_connect
always calls intoself.request
think the simplest solution to avoid a breaking change is:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably want that in the _request() check, but probably still want to limit it to http/https in the redirects, and disable redirects in _ws_connect() (as mentioned above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. So it seems we need to different constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should include wss:// as well as this breaks discord.py entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably lost track of this, maybe nobody got round to making the change discussed above.