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

Handle Location header relative URI #1240

Merged
merged 3 commits into from
Nov 30, 2018

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Nov 29, 2018

This addresses 1st part of problem for #1239 (the location header in the sample given was /[email protected]?module, not a full path)

Refs:
https://tools.ietf.org/html/rfc7231#page-68
https://tools.ietf.org/html/rfc3986#section-4.2
https://tools.ietf.org/html/rfc3986#section-3.3

@kevinkassimo kevinkassimo changed the title [WIP] Handle Location header relative URI Handle Location header relative URI Nov 29, 2018
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the fix. One comment...

src/http_util.rs Outdated
@@ -26,6 +26,37 @@ pub fn get_client() -> Client<Connector, hyper::Body> {
Client::builder().build(c)
}

// Construct the next uri based on base uri and location header fragment
// https://tools.ietf.org/html/rfc3986#section-4.2
fn maybe_uri_from_location(base_uri: &Uri, location: &str) -> Option<Uri> {
Copy link
Member

Choose a reason for hiding this comment

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

  1. I think it's better to call this resolve_uri_from_location
  2. Can you use triple slashes for the comments, so it shows up in rustdoc?
  3. Also please describe in the docs when the "None" case occurs.

Copy link
Member

Choose a reason for hiding this comment

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

(We haven't been doing triple slash comments for Rust - but I will be trying to fix that in the future - as we move towards releasing a crate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed the Option wrapper since it is actually not needed with current design.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 286e76d into denoland:master Nov 30, 2018
ry added a commit to ry/deno that referenced this pull request Nov 30, 2018
- Allow async functions in REPL (denoland#1233)
- Handle Location header relative URI (denoland#1240)
- Add deno.readAll() (denoland#1234)
- Add Process.output (denoland#1235)
- Upgrade to TypeScript 3.2.1
- Upgrade crates: tokio 0.1.13, hyper 0.12.16, ring 0.13.5
@ry ry mentioned this pull request Nov 30, 2018
ry added a commit that referenced this pull request Dec 1, 2018
- Allow async functions in REPL (#1233)
- Handle Location header relative URI (#1240)
- Add deno.readAll() (#1234)
- Add Process.output (#1235)
- Upgrade to TypeScript 3.2.1
- Upgrade crates: tokio 0.1.13, hyper 0.12.16, ring 0.13.5
@kevinkassimo kevinkassimo deleted the redirect/relative_location branch December 27, 2019 07:51
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.

None yet

2 participants