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

Separate tar archive and tar flags #492

Merged
merged 2 commits into from
Apr 18, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 7 additions & 2 deletions src/archive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,15 @@ impl CompressionMethod {
}
}

pub fn is_enabled(self, tar_enabled: bool, zip_enabled: bool) -> bool {
pub fn is_enabled(
self,
tar_enabled: bool,
deantvv marked this conversation as resolved.
Show resolved Hide resolved
tar_archive_enabled: bool,
zip_enabled: bool,
) -> bool {
match self {
CompressionMethod::TarGz => tar_enabled,
CompressionMethod::Tar => tar_enabled,
CompressionMethod::TarGz => tar_archive_enabled,
CompressionMethod::Zip => zip_enabled,
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,14 @@ pub struct CliArgs {
#[structopt(short = "o", long = "overwrite-files")]
pub overwrite_files: bool,

/// Enable tar archive generation
/// Enable tar generation
deantvv marked this conversation as resolved.
Show resolved Hide resolved
#[structopt(short = "r", long = "enable-tar")]
pub enable_tar: bool,

/// Enable tar archive generation
deantvv marked this conversation as resolved.
Show resolved Hide resolved
#[structopt(short = "g", long = "enable-tar-archive")]
pub enable_tar_archive: bool,

/// Enable zip archive generation
///
/// WARNING: Zipping large directories can result in out-of-memory exception
Expand Down
4 changes: 3 additions & 1 deletion src/listing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ pub fn directory_listing(
show_qrcode: bool,
upload_route: String,
tar_enabled: bool,
tar_archive_enabled: bool,
zip_enabled: bool,
dirs_first: bool,
hide_version_footer: bool,
Expand Down Expand Up @@ -330,7 +331,7 @@ pub fn directory_listing(
}

if let Some(compression_method) = query_params.download {
if !compression_method.is_enabled(tar_enabled, zip_enabled) {
if !compression_method.is_enabled(tar_enabled, tar_archive_enabled, zip_enabled) {
return Ok(ServiceResponse::new(
req.clone(),
HttpResponse::Forbidden()
Expand Down Expand Up @@ -413,6 +414,7 @@ pub fn directory_listing(
&encoded_dir,
breadcrumbs,
tar_enabled,
tar_archive_enabled,
zip_enabled,
hide_version_footer,
)
Expand Down
8 changes: 7 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ pub struct MiniserveConfig {
/// Enable upload to override existing files
pub overwrite_files: bool,

/// If false, creation of tar archives is disabled
/// If false, creation of tar is disabled
pub tar_enabled: bool,

/// If false, creation of tar archives is disabled
pub tar_archive_enabled: bool,

/// If false, creation of zip archives is disabled
pub zip_enabled: bool,

Expand Down Expand Up @@ -161,6 +164,7 @@ impl MiniserveConfig {
show_qrcode: args.qrcode,
file_upload: args.file_upload,
tar_enabled: args.enable_tar,
tar_archive_enabled: args.enable_tar_archive,
zip_enabled: args.enable_zip,
dirs_first: args.dirs_first,
title: args.title,
Expand Down Expand Up @@ -411,6 +415,7 @@ fn configure_app(app: &mut web::ServiceConfig, conf: &MiniserveConfig) {
let show_qrcode = conf.show_qrcode;
let file_upload = conf.file_upload;
let tar_enabled = conf.tar_enabled;
let tar_archive_enabled = conf.tar_archive_enabled;
let zip_enabled = conf.zip_enabled;
let dirs_first = conf.dirs_first;
let hide_version_footer = conf.hide_version_footer;
Expand Down Expand Up @@ -453,6 +458,7 @@ fn configure_app(app: &mut web::ServiceConfig, conf: &MiniserveConfig) {
show_qrcode,
u_r.clone(),
tar_enabled,
tar_archive_enabled,
zip_enabled,
dirs_first,
hide_version_footer,
Expand Down
5 changes: 3 additions & 2 deletions src/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub fn page(
encoded_dir: &str,
breadcrumbs: Vec<Breadcrumb>,
tar_enabled: bool,
tar_archive_enabled: bool,
zip_enabled: bool,
hide_version_footer: bool,
) -> Markup {
Expand Down Expand Up @@ -94,10 +95,10 @@ pub fn page(
}
}
div.toolbar {
@if tar_enabled || zip_enabled {
@if tar_enabled || tar_archive_enabled || zip_enabled {
div.download {
@for compression_method in CompressionMethod::iter() {
@if compression_method.is_enabled(tar_enabled, zip_enabled) {
@if compression_method.is_enabled(tar_enabled, tar_archive_enabled, zip_enabled) {
(archive_button(compression_method, sort_method, sort_order))
}
}
Expand Down
41 changes: 41 additions & 0 deletions tests/archive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,44 @@ fn archives_are_disabled(tmpdir: TempDir, port: u16) -> Result<(), Error> {

Ok(())
}

#[rstest]
deantvv marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

If you wanna do something cool here, you could use rstest for test parameterization to test -p and -g with very little test code overhead. Want to give that a try?

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, on second thought, I want to more properly clean up these tests anyhow. I'll just do that after merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try in maybe another PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, if you like, that'd be much appreciated. So what I'm thinking here: Have a single parametrized test in tests/archive.rs that would test for all possible combinations of compression flags whether the path is reachable whether the links are being rendered. Thanks to rstest, we'd end up with a bunch of separately generated distinct tests. I think that'd be rather clean and make quite a bit of sense. Wanna give it a shot?

Take a look at: https://docs.rs/rstest/0.7.0/rstest/attr.rstest.html#test-parametrized-cases

fn test_tar_archives(tmpdir: TempDir, port: u16) -> Result<(), Error> {
let mut child = Command::cargo_bin("miniserve")?
.arg(tmpdir.path())
.arg("-p")
.arg(port.to_string())
.arg("-g")
.stdout(Stdio::null())
.spawn()?;

sleep(Duration::from_secs(1));

// Ensure the links to the tar archive exists and tar not exists
Copy link
Owner

Choose a reason for hiding this comment

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

One small thing I just found:
// Ensure the links to the compressed tar archive exists and link to plain tar doesn't exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to fix this. But since you already merge this, should I still change this?

let body = reqwest::blocking::get(format!("http://localhost:{}", port).as_str())?
.error_for_status()?;
let parsed = Document::from_read(body)?;
assert!(parsed.find(Text).any(|x| x.text() == "Download .tar.gz"));
assert!(parsed.find(Text).all(|x| x.text() != "Download .tar"));

// Try to download, only tar_gz should works
Copy link
Owner

Choose a reason for hiding this comment

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

// Try to download, only tar.gz should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

assert_eq!(
reqwest::blocking::get(format!("http://localhost:{}/?download=tar_gz", port).as_str())?
.status(),
StatusCode::OK
);
assert_eq!(
reqwest::blocking::get(format!("http://localhost:{}/?download=tar", port).as_str())?
.status(),
StatusCode::FORBIDDEN
);
assert_eq!(
reqwest::blocking::get(format!("http://localhost:{}/?download=zip", port).as_str())?
.status(),
StatusCode::FORBIDDEN
);

child.kill()?;

Ok(())
}