Skip to content

Improve link fallback #582

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Improve link fallback #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08526f9
Make missing CRATESFYI_PREFIX easier to debug
Zexbe Feb 2, 2020
777b37c
Fallback in more situations without baskslash
Zexbe Feb 2, 2020
d561b6c
Fix, and Fallback with ext-less files
Zexbe Feb 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/web/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl CratesfyiHandler {

let shared_resources = Self::chain(&pool_factory, rustdoc::SharedResourceHandler);
let router_chain = Self::chain(&pool_factory, routes.iron_router());
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").unwrap()).join("public_html");
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html");
Copy link
Member

Choose a reason for hiding this comment

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

What do you think, is this overkill?

Suggested change
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html");
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX")
.expect("the CRATESFYI_PREFIX environment variable is not set, \
see https://github.com/rust-lang/docs.rs/wiki/Developing-without-docker-compose \
for instructions on developing locally")
).join("public_html");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue I feel with putting the link on the expect is that it is weird for this one to have it, and not the other ones, but if all of them have it, it gets pretty ugly, so maybe it needs to just check all the environment variables at the start, and print an error message, but I don't think it should be put into this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("CRATESFYI_PREFIX environment variable does not exists")).join("public_html");
let prefix = PathBuf::from(env::var("CRATESFYI_PREFIX").expect("the CRATESFYI_PREFIX environment variable is not set")).join("public_html");

let static_handler = Static::new(prefix)
.cache(Duration::from_secs(STATIC_FILE_CACHE_DURATION));

Expand Down
42 changes: 26 additions & 16 deletions src/web/rustdoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use time;
use iron::Handler;
use utils;

const DIR_DEFAULT_FILE: &str = "index.html";

#[derive(Debug)]
struct RustdocPage {
Expand Down Expand Up @@ -238,26 +239,28 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult<Response> {
return Ok(super::redirect(canonical));
}

let path = {
let mut path = req_path.join("/");
if path.ends_with('/') {
req_path.pop(); // get rid of empty string
path.push_str("index.html");
req_path.push("index.html");
}
path
let mut path = req_path.join("/");

//If it is a directory, we default to looking
if path.ends_with("/") {
path.push_str(DIR_DEFAULT_FILE);
req_path.push(DIR_DEFAULT_FILE);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the same behavior, it will turn /crate/version/ into /crate/version//index.html.

This will mess up the redirect to latest version, which unfortunately only has unit tests in #570.

Since that's the case, I'm not comfortable merging until either #570 is merged or I get a chance to test locally which will probably be a few days.

If you instead change only the from_path block (line 251), I would be more comfortable with the changes.

}

if !path.ends_with(".html") {
if let Some(file) = File::from_path(&conn, &path) {
return Ok(file.serve());
};
path.push_str("/");
path.push_str(DIR_DEFAULT_FILE);
req_path.push(DIR_DEFAULT_FILE);
};

let file = match File::from_path(&conn, &path) {
Some(f) => f,
Some(file) => file,
None => return Err(IronError::new(Nope::ResourceNotFound, status::NotFound)),
};

// serve file directly if it's not html
if !path.ends_with(".html") {
return Ok(file.serve());
}

let mut content = RustdocPage::default();

let file_content = ctry!(String::from_utf8(file.0.content));
Expand Down Expand Up @@ -315,7 +318,7 @@ fn path_for_version(req_path: &[&str], target_name: &str, conn: &Connection) ->
return req_path[3..].join("/");
}
// this page doesn't exist in the latest version
let search_item = if *req_path.last().unwrap() == "index.html" {
let search_item = if *req_path.last().unwrap() == DIR_DEFAULT_FILE {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you made this change?

// this is a module
req_path[req_path.len() - 2]
} else {
Expand Down Expand Up @@ -438,18 +441,24 @@ mod test {
db.fake_release()
.name("buggy").version("0.1.0")
.build_result_successful(true)
.rustdoc_file("settings.html", b"some data")
.rustdoc_file("settings.html", b"some setting data")
.rustdoc_file("all.html", b"some data 2")
.rustdoc_file("file_without_ext", b"some data 3")
.rustdoc_file("some_module/directory/index.html", b"some data 3")
.rustdoc_file("some_module/file_without_ext", b"some data 3")
Copy link
Member

Choose a reason for hiding this comment

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

Please also test /directory.html/index.html, which I don't think will work currently.

.create()?;
db.fake_release()
.name("buggy").version("0.2.0")
.build_result_successful(false)
.create()?;
let web = env.frontend();
assert_success("/", web)?;
assert_success("/crate/buggy/0.1.0", web)?;
assert_success("/crate/buggy/0.1.0/", web)?;
assert_success("/buggy/0.1.0/settings.html", web)?;
assert_success("/buggy/0.1.0/all.html", web)?;
assert_success("/buggy/0.1.0/some_module/file_without_ext", web)?;
assert_success("/buggy/0.1.0/some_module/directory", web)?;
Ok(())
});
}
Expand Down Expand Up @@ -484,6 +493,7 @@ mod test {
.rustdoc_file("dummy/index.html", b"some content")
.rustdoc_file("all.html", b"html")
.default_target(target).create()?;

let base = "/dummy/0.3.0/dummy/";
assert_success(base, web)?;
assert_redirect("/dummy/0.3.0/x86_64-unknown-linux-gnu/dummy/", base, web)?;
Expand Down