Skip to content
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

ws client redirections #397

Merged
merged 43 commits into from
Oct 5, 2021
Merged

ws client redirections #397

merged 43 commits into from
Oct 5, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Jun 30, 2021

Closes #339

@dvdplm
Copy link
Contributor

dvdplm commented Aug 23, 2021

Left a few thoughts!

ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some grumbles

ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 changed the title [WIP] ws client redirections ws client redirections Sep 30, 2021
@niklasad1 niklasad1 marked this pull request as ready for review September 30, 2021 14:35
test-utils/Cargo.toml Show resolved Hide resolved
test-utils/src/types.rs Show resolved Hide resolved
test-utils/src/types.rs Outdated Show resolved Hide resolved
ws-client/src/tests.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
ws-client/src/transport.rs Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member Author

Weird, especially that the hostname is 0. I would expect it to be 127.0.0.1 ^

Ah, I figured it out basically 0just prints 0 this was a thiserror nit 🙈

ws-client/src/transport.rs Outdated Show resolved Hide resolved
// Relative URI.
else {
// Replace the entire path_and_query if `location` starts with `/` or `//`.
if location.starts_with('/') {
Copy link
Member Author

@niklasad1 niklasad1 Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//cc @maciejhirsz is it fair to assume that if the initial request included a query such as ws://example.com/page?name=hi then the server will include it in the re-direction location header?

pin-project = "1"
thiserror = "1"
url = "2"
http = "0.2"
Copy link
Member Author

@niklasad1 niklasad1 Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to the http crate instead for URI parsing because it supports both absolute URI and relative resource URI

Useful when getting the location header back...

However, it doesn't support to get the default port and it's put in an Option<Port> so you can't really tell whether the port was invalid or no port was provided....

ws-client/src/tests.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 requested a review from maciejhirsz October 4, 2021 16:41
if location.starts_with('/') {
target.path_and_query = location;
} else {
match target.path_and_query.rfind('/') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//cc @maciejhirsz I had to do this instead of Path::join because on Windows the path is joined by \ which is a faulty URI relative resource.

This will work for both // and / FWIW.

if is_upgrade_request(&req) {
log::debug!("{:?}", req);

match req.uri().path() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh okay the above description makes sense after reading this! 👍

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty clean!

@niklasad1 niklasad1 merged commit 94c881b into master Oct 5, 2021
@niklasad1 niklasad1 deleted the na-ws-client-redirections branch October 5, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ws client]: support HTTP redirections.
4 participants