From 57b86ea064ffb4cb8d9644b7b4dfdae5a9e9c9b2 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 10:37:37 -0300 Subject: [PATCH 01/15] chore: Introduce `cargo stele` for dev/CI utils See the comments at the top of `./cargo-stele` for more details. Basically, this is just an example. `just`[1] is probably better. 1. https://github.com/casey/just --- .github/workflows/ci.yml | 2 +- justfile | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9a805b1..f26a590 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.STELE_GITHUB_TOKEN }} - - name: Run just test + - name: Run tests run: just test lints: diff --git a/justfile b/justfile index 4b31fca..dcf5777 100644 --- a/justfile +++ b/justfile @@ -9,7 +9,7 @@ lint: format clippy # Format code format: - cargo fmt --all -- --check + cargo fmt --all -- --check # Run all tests test: @@ -27,4 +27,4 @@ ci: lint test bench # Run all benchmarks bench: - cargo bench \ No newline at end of file + cargo bench From 4cb71ce29c0a0847ae9bc85baf6f80d2915be59b Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 12:03:10 -0300 Subject: [PATCH 02/15] chore: Refactor linting into `lib.rs` See `lib.rs` for detailed explanation. The idea here is to centralise lint requirements as much as possible. Avoiding unnecessary repetition. Perhaps it could be thought of as a bit like the "cascade" in CSS. There's a base "pendantic" requirement for as much lint feedback as possible. Then crate-wide exceptions are made. Then file-wide exceptions are added. And finally function-specific exceptions are made on a case by case bases. Note that by default lints will just produce warnings. Only in CI will warnings be converted to failures. --- README.md | 1 - benches/git_benchmark.rs | 2 -- src/lib.rs | 74 +++++++++++++++++++++++++++++++++++++--- src/main.rs | 58 +++++-------------------------- src/server/git.rs | 11 +++--- src/utils.rs | 1 + src/utils/cli.rs | 58 +++++++++++++++++++++++++++++++ src/utils/git.rs | 3 -- 8 files changed, 143 insertions(+), 65 deletions(-) create mode 100644 src/utils/cli.rs diff --git a/README.md b/README.md index d6feef7..d9d20b6 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,6 @@ Stele is a system for distributing, preserving, and authenticating laws. - `test`: Run all tests - On windows, especially, you may wish to run just through the nu shell, which can be done by calling all commands with the `--shell` command, e.g. `just --shell nu lint`. - ## Q&A - Why do we suggest NuShell? - NuShell is almost as fast on windows as cmd, but is compattible with bash. If you do not use NuShell on windows, you will need to make sure Git Bash is installed. If you have performance issues, consider switching to Nu. diff --git a/benches/git_benchmark.rs b/benches/git_benchmark.rs index 22ef98a..059d597 100644 --- a/benches/git_benchmark.rs +++ b/benches/git_benchmark.rs @@ -1,8 +1,6 @@ //! benchmark for git utils #![allow(clippy::self_named_module_files)] -#![allow(clippy::std_instead_of_alloc)] #![allow(clippy::implicit_return)] -#![allow(clippy::multiple_crate_versions)] #![allow(clippy::expect_used)] #![allow(missing_docs)] diff --git a/src/lib.rs b/src/lib.rs index 33db803..8c43329 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,11 +9,75 @@ //! publish the law. The Code of Hammurabi, one of the earliest preserved //! written laws, was published on a Stele in ~1750 BCE and is still readable //! nearly four millennia later. -#![allow(clippy::self_named_module_files)] -#![allow(clippy::std_instead_of_alloc)] -#![allow(clippy::implicit_return)] -#![allow(clippy::multiple_crate_versions)] -#![allow(clippy::std_instead_of_core)] + +// ========================================================================= +// Canonical lints for whole crate +// ========================================================================= +// Official docs: +// https://doc.rust-lang.org/nightly/clippy/lints.html +// Useful app to lookup full details of individual lints: +// https://rust-lang.github.io/rust-clippy/master/index.html +// +// We set base lints to give the fullest, most pedantic feedback possible. +// Though we prefer that they are just warnings during development so that build-denial +// is only enforced in CI. +// +#![warn( + // `clippy::all` is already on by default. It implies the following: + // clippy::correctness code that is outright wrong or useless + // clippy::suspicious code that is most likely wrong or useless + // clippy::complexity code that does something simple but in a complex way + // clippy::perf code that can be written to run faster + // clippy::style code that should be written in a more idiomatic way + clippy::all, + + // It's always good to write as much documentation as possible + missing_docs, + + // > clippy::pedantic lints which are rather strict or might have false positives + clippy::pedantic, + + // > new lints that are still under development" + // (so "nursery" doesn't mean "Rust newbies") + clippy::nursery, + + // > The clippy::cargo group gives you suggestions on how to improve your Cargo.toml file. + // > This might be especially interesting if you want to publish your crate and are not sure + // > if you have all useful information in your Cargo.toml. + clippy::cargo +)] +// > The clippy::restriction group will restrict you in some way. +// > If you enable a restriction lint for your crate it is recommended to also fix code that +// > this lint triggers on. However, those lints are really strict by design and you might want +// > to #[allow] them in some special cases, with a comment justifying that. +#![allow(clippy::blanket_clippy_restriction_lints)] +#![warn(clippy::restriction)] +// +// +// ========================================================================= +// Individually blanket-allow single lints relevant to this whole crate +// ========================================================================= +#![allow( + // This is idiomatic Rust + clippy::implicit_return, + + // Multiple deps are currently pinning `hermit-abi` — December 2022 + clippy::multiple_crate_versions, + + // We're not interested in becoming no-std compatible + clippy::std_instead_of_alloc, + clippy::std_instead_of_core, + + // TODO: But I think the mod.rs is more conventional — @tombh + clippy::self_named_module_files, + + // Although performance is of course important for this application, it is not currently + // such that it would benefit from explicit inline suggestions. Besides, not specifying + // `#[inline]` doesn't mean that a function won't be inlined. And if performance does start + // to become a problem, there are other avenues to explore before deciding on which functions + // would benefit from explicit inlining. + clippy::missing_inline_in_public_items +)] pub mod server; pub mod utils; diff --git a/src/main.rs b/src/main.rs index e4b754d..f6f95ba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,55 +1,13 @@ -//! Stele commandline. -#![allow(clippy::self_named_module_files)] -#![allow(clippy::std_instead_of_alloc)] -#![allow(clippy::implicit_return)] -#![allow(clippy::multiple_crate_versions)] -#![allow(clippy::exhaustive_structs)] +//! Just main(). Keep as small as possible. -use clap::Parser; -use std::path::Path; -use stele::server::git::serve_git; -use stele::utils::library::find_library_path; +// The `main.rs` file is special in Rust. +// So attributes here have no affect on the main codebase. If the file remains minimal we can just +// blanket allow lint groups. +#![allow(clippy::cargo)] +#![allow(clippy::restriction)] -/// Stele is currently just a simple git server. -/// run from the library directory or pass -/// path to library. -#[derive(Parser)] -#[command(author, version, about, long_about = None)] -struct Cli { - /// Path to the Stele library. Defaults to cwd. - #[arg(short, long, default_value_t = String::from(".").to_owned())] - library_path: String, - /// Stele cli subcommands - #[command(subcommand)] - subcommands: Subcommands, -} - -/// -#[derive(Clone, clap::Subcommand)] -enum Subcommands { - /// Serve git repositories in the Stele library - Git { - /// Port on which to serve the library. - #[arg(short, long, default_value_t = 8080)] - port: u16, - }, -} +use stele::utils::cli::run; -#[allow(clippy::print_stdout)] fn main() -> std::io::Result<()> { - let cli = Cli::parse(); - let library_path_wd = Path::new(&cli.library_path); - let library_path = if let Ok(lpath) = find_library_path(library_path_wd) { - lpath - } else { - println!( - "error: could not find `.stele` folder in `{}` or any parent directory", - &cli.library_path - ); - std::process::exit(1); - }; - - match cli.subcommands { - Subcommands::Git { port } => serve_git(&cli.library_path, library_path, port), - } + run() } diff --git a/src/server/git.rs b/src/server/git.rs index bd8ff9c..9095fcf 100644 --- a/src/server/git.rs +++ b/src/server/git.rs @@ -1,7 +1,11 @@ //! Legacy git microserver. -//! -//! Will be deprecated upon completion of Publish Server migration to rust. -#![allow(clippy::exhaustive_structs)] + +#![allow( + // Will be deprecated upon completion of Publish Server migration to rust. + clippy::exhaustive_structs, + // Unused asyncs are the norm in Actix route definition files + clippy::unused_async +)] use crate::utils::git::Repo; use crate::utils::http::get_contenttype; @@ -28,7 +32,6 @@ fn clean_path(path: &str) -> String { /// Return the content in the stele library in the `{namespace}/{name}` /// repo at the `commitish` commit at the `remainder` path. /// Return 404 if any are not found or there are any errors. -#[allow(clippy::unused_async)] #[get("/{namespace}/{name}/{commitish}{remainder:/+([^{}]*?)?/*}")] async fn get_blob( path: web::Path<(String, String, String, String)>, diff --git a/src/utils.rs b/src/utils.rs index 48eb62f..7c99583 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,5 +1,6 @@ //! The utils module contains utility functions and structs. +pub mod cli; pub mod git; pub mod http; pub mod library; diff --git a/src/utils/cli.rs b/src/utils/cli.rs new file mode 100644 index 0000000..72fa5ef --- /dev/null +++ b/src/utils/cli.rs @@ -0,0 +1,58 @@ +//! Running the CLI + +// Allow exits because in this file we ideally handle all errors with known exit codes +#![allow(clippy::exit)] + +use crate::server::git::serve_git; +use crate::utils::library::find_library_path; +use clap::Parser; +use std::path::Path; + +/// Stele is currently just a simple git server. +/// run from the library directory or pass +/// path to library. +#[derive(Parser)] +#[command(author, version, about, long_about = None)] +struct Cli { + /// Path to the Stele library. Defaults to cwd. + #[arg(short, long, default_value_t = String::from(".").to_owned())] + library_path: String, + /// Stele cli subcommands + #[command(subcommand)] + subcommands: Subcommands, +} + +/// +#[derive(Clone, clap::Subcommand)] +enum Subcommands { + /// Serve git repositories in the Stele library + Git { + /// Port on which to serve the library. + #[arg(short, long, default_value_t = 8080)] + port: u16, + }, +} + +/// Main entrypoint to application +/// +/// # Errors +/// TODO: This function should not return errors +#[allow(clippy::print_stdout)] +#[inline] +pub fn run() -> std::io::Result<()> { + let cli = Cli::parse(); + let library_path_wd = Path::new(&cli.library_path); + let library_path = if let Ok(lpath) = find_library_path(library_path_wd) { + lpath + } else { + println!( + "error: could not find `.stele` folder in `{}` or any parent directory", + &cli.library_path + ); + std::process::exit(1); + }; + + match cli.subcommands { + Subcommands::Git { port } => serve_git(&cli.library_path, library_path, port), + } +} diff --git a/src/utils/git.rs b/src/utils/git.rs index 4032af6..14f8746 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -19,7 +19,6 @@ pub struct Repo { impl fmt::Debug for Repo { #[inline] - #[allow(clippy::implicit_return)] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, @@ -38,7 +37,6 @@ impl Repo { /// Will return `Err` if git repository does not exist at `{namespace}/{name}` /// in library, or if there is something wrong with the git repository. - #[allow(clippy::implicit_return)] #[inline] pub fn new(lib_path: &Path, namespace: &str, name: &str) -> Result { let lib_path_str = lib_path.to_string_lossy(); @@ -65,7 +63,6 @@ impl Repo { /// /// Will return `Err` if `commitish` does not exist in repo, if a blob does /// not exist in commit at `path`, or if there is a problem with reading repo. - #[allow(clippy::implicit_return)] #[inline] pub fn get_bytes_at_path(&self, commitish: &str, path: &str) -> anyhow::Result> { let base_revision = format!("{commitish}:{path}"); From d7d80dfd4a5a09f17572ce08cae07c2a171d95f9 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 12:23:25 -0300 Subject: [PATCH 03/15] chore: Remove linting definitions from `.vscode` Lints are now formalised in the source code itself. But it's still good to give VSCode users the headstart of defaulting to Clipp over `cargo check`. I added a `etc/NOTES.md` to keep a record of the pendantic VSCode settings. I personally use something like as my default Neovim config. But having a `etc/NOTES.md` might not be something you want. --- etc/NOTES.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 etc/NOTES.md diff --git a/etc/NOTES.md b/etc/NOTES.md new file mode 100644 index 0000000..f3d1520 --- /dev/null +++ b/etc/NOTES.md @@ -0,0 +1,29 @@ +# Project Notes + +## Pedantic Rust Editor Settings +If you would like to make your editor always give you the most verbose feedback in any Rust project, you can use something like this. It can be used as-is in VSCode, or converted to something similar in your editor of choice. + +```json +{ + "rust-analyzer.checkOnSave.overrideCommand": [ + "cargo", + "clippy", + "--message-format=json", + "--all-targets", + "--all-features", + "--", + "-W", + "clippy::all", + "-W", + "clippy::pedantic", + "-W", + "clippy::restriction", + "-W", + "clippy::nursery", + "-W", + "clippy::cargo", + "-W", + "missing_docs" + ], +} +``` From af91bcc2cb9c6dc4c19a1084a96612ec33a18864 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 12:33:55 -0300 Subject: [PATCH 04/15] chore: Remove `#[inline]`s Copied comment from `lib.rs` linting: Although performance is of course important for this application, it is not currently such that it would benefit from explicit inline suggestions. besides, not specifying `[inline]` doesn't mean that a function won't be inlined. And if performance does start to become a problem, there are other avenues to explore before deciding on which functions would benefit from explicit inlining. --- src/utils/cli.rs | 1 - src/utils/git.rs | 3 --- src/utils/http.rs | 1 - src/utils/library.rs | 1 - 4 files changed, 6 deletions(-) diff --git a/src/utils/cli.rs b/src/utils/cli.rs index 72fa5ef..7584c7c 100644 --- a/src/utils/cli.rs +++ b/src/utils/cli.rs @@ -38,7 +38,6 @@ enum Subcommands { /// # Errors /// TODO: This function should not return errors #[allow(clippy::print_stdout)] -#[inline] pub fn run() -> std::io::Result<()> { let cli = Cli::parse(); let library_path_wd = Path::new(&cli.library_path); diff --git a/src/utils/git.rs b/src/utils/git.rs index 14f8746..c34d851 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -18,7 +18,6 @@ pub struct Repo { } impl fmt::Debug for Repo { - #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, @@ -37,7 +36,6 @@ impl Repo { /// Will return `Err` if git repository does not exist at `{namespace}/{name}` /// in library, or if there is something wrong with the git repository. - #[inline] pub fn new(lib_path: &Path, namespace: &str, name: &str) -> Result { let lib_path_str = lib_path.to_string_lossy(); let repo_path = format!("{lib_path_str}/{namespace}/{name}"); @@ -63,7 +61,6 @@ impl Repo { /// /// Will return `Err` if `commitish` does not exist in repo, if a blob does /// not exist in commit at `path`, or if there is a problem with reading repo. - #[inline] pub fn get_bytes_at_path(&self, commitish: &str, path: &str) -> anyhow::Result> { let base_revision = format!("{commitish}:{path}"); for postfix in ["", "/index.html", ".html", "index.html"] { diff --git a/src/utils/http.rs b/src/utils/http.rs index 87ed956..29ff459 100644 --- a/src/utils/http.rs +++ b/src/utils/http.rs @@ -7,7 +7,6 @@ use std::path::Path; /// for the content at `path`. If there is no extension, we assume it is /// html. If the extension cannot be converted to a str, then we return /// octet stream. -#[inline] #[must_use] pub fn get_contenttype(path: &str) -> ContentType { // let mimetype = get_mimetype(blob_path); diff --git a/src/utils/library.rs b/src/utils/library.rs index 8b0c110..8a18f7b 100644 --- a/src/utils/library.rs +++ b/src/utils/library.rs @@ -6,7 +6,6 @@ use std::path::{Path, PathBuf}; /// /// # Errors /// Error if the path doesn't exist or isn't inside a Stele library. -#[inline] pub fn find_library_path(path: &Path) -> anyhow::Result { let abs_path = path.canonicalize()?; for working_path in abs_path.ancestors() { From b7c011a2e55d44fe3b97d0976f206017815baee4 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 13:16:13 -0300 Subject: [PATCH 05/15] chore: Move integration-style tests to tests/ Any tests amongst src/* files are generally unit-style tests. Of course this isn't a strict requirement, just Rust convention. The fact that these tests depended on fixtures made it quite clear that they are integration tests. --- benches/git_benchmark.rs | 20 +-- src/utils/git.rs | 131 ------------------ src/utils/library.rs | 65 --------- tests/basic/gitrepo_test.rs | 93 +++++++++++++ tests/basic/library_test.rs | 41 ++++++ tests/basic/mod.rs | 2 + tests/basic_test.rs | 5 + tests/common/mod.rs | 21 +++ {test => tests/fixtures}/library/.stele/.keep | 0 .../fixtures}/library/test/law-html/HEAD | 0 .../fixtures}/library/test/law-html/config | 0 .../library/test/law-html/description | 0 .../test/law-html/hooks/applypatch-msg.sample | 0 .../test/law-html/hooks/commit-msg.sample | 0 .../law-html/hooks/fsmonitor-watchman.sample | 0 .../test/law-html/hooks/post-update.sample | 0 .../test/law-html/hooks/pre-applypatch.sample | 0 .../test/law-html/hooks/pre-commit.sample | 0 .../law-html/hooks/pre-merge-commit.sample | 0 .../test/law-html/hooks/pre-push.sample | 0 .../test/law-html/hooks/pre-rebase.sample | 0 .../test/law-html/hooks/pre-receive.sample | 0 .../law-html/hooks/prepare-commit-msg.sample | 0 .../library/test/law-html/hooks/update.sample | 0 .../library/test/law-html/info/exclude | 0 .../2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c | Bin .../b6/4fe7f7ea66c117596363e1179e30892a40572c | 0 .../c5/3c5c71365e1d88752ed223b14f437d698c8806 | Bin .../test/law-html/objects/info/commit-graph | Bin .../library/test/law-html/objects/info/packs | 0 ...5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx | Bin ...ff518cf56dc47a9481ab25de6b0fff831b5ce.pack | Bin .../library/test/law-html/packed-refs | 0 33 files changed, 166 insertions(+), 212 deletions(-) create mode 100644 tests/basic/gitrepo_test.rs create mode 100644 tests/basic/library_test.rs create mode 100644 tests/basic/mod.rs create mode 100644 tests/basic_test.rs create mode 100644 tests/common/mod.rs rename {test => tests/fixtures}/library/.stele/.keep (100%) rename {test => tests/fixtures}/library/test/law-html/HEAD (100%) rename {test => tests/fixtures}/library/test/law-html/config (100%) rename {test => tests/fixtures}/library/test/law-html/description (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/applypatch-msg.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/commit-msg.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/fsmonitor-watchman.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/post-update.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-applypatch.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-commit.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-merge-commit.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-push.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-rebase.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/pre-receive.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/prepare-commit-msg.sample (100%) rename {test => tests/fixtures}/library/test/law-html/hooks/update.sample (100%) rename {test => tests/fixtures}/library/test/law-html/info/exclude (100%) rename {test => tests/fixtures}/library/test/law-html/objects/2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c (100%) rename {test => tests/fixtures}/library/test/law-html/objects/b6/4fe7f7ea66c117596363e1179e30892a40572c (100%) rename {test => tests/fixtures}/library/test/law-html/objects/c5/3c5c71365e1d88752ed223b14f437d698c8806 (100%) rename {test => tests/fixtures}/library/test/law-html/objects/info/commit-graph (100%) rename {test => tests/fixtures}/library/test/law-html/objects/info/packs (100%) rename {test => tests/fixtures}/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx (100%) rename {test => tests/fixtures}/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.pack (100%) rename {test => tests/fixtures}/library/test/law-html/packed-refs (100%) diff --git a/benches/git_benchmark.rs b/benches/git_benchmark.rs index 059d597..33fa0a9 100644 --- a/benches/git_benchmark.rs +++ b/benches/git_benchmark.rs @@ -5,28 +5,16 @@ #![allow(missing_docs)] use criterion::{criterion_group, criterion_main, Criterion}; -use std::env::current_exe; use std::fs::create_dir_all; use std::path::PathBuf; use std::sync::Once; use stele::utils::git::Repo; -/// get the path to the test library at `$REPO_ROOT/test/library`. +/// get the path to the test library at `$REPO_ROOT/tests/fixtures/library`. fn get_test_library_path() -> PathBuf { - let mut library_path = current_exe() - .expect("Something went wrong getting the library path") - .parent() - .expect("Something went wrong getting the library path") - .parent() - .expect("Something went wrong getting the library path") - .parent() - .expect("Something went wrong getting the library path") - .parent() - .expect("Something went wrong getting the library path") - .to_owned(); - library_path.push("test"); - library_path.push("library"); - library_path + let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + path.push("tests/fixtures/library"); + path } /// ensure `initialize` function, below, is only called once diff --git a/src/utils/git.rs b/src/utils/git.rs index c34d851..00108a9 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -81,134 +81,3 @@ impl Repo { Err(anyhow::anyhow!("Doesn't exist")) } } - -#[allow(clippy::unwrap_used)] -#[allow(clippy::string_slice)] -#[allow(clippy::indexing_slicing)] -#[cfg(test)] -mod tests { - use crate::utils::git::Repo; - use std::env::current_exe; - use std::fs::create_dir_all; - use std::path::PathBuf; - use std::sync::Once; - - static INIT: Once = Once::new(); - - pub fn initialize() { - INIT.call_once(|| { - let repo_path = get_test_library_path().join(PathBuf::from("test/law-html")); - let heads_path = repo_path.join(PathBuf::from("refs/heads")); - create_dir_all(heads_path).unwrap(); - let tags_path = repo_path.join(PathBuf::from("refs/tags")); - create_dir_all(tags_path).unwrap(); - }); - } - - fn get_test_library_path() -> PathBuf { - let mut library_path = current_exe() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .to_owned(); - library_path.push("test"); - library_path.push("library"); - library_path - } - - #[test] - fn test_get_bytes_at_path_when_empty_path_expect_index_html() { - initialize(); - let test_library_path = get_test_library_path(); - let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "") - .unwrap(); - let expected = ""; - assert_eq!( - &core::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected - ); - } - - #[test] - fn test_get_bytes_at_path_when_full_path_expect_data() { - initialize(); - let test_library_path = get_test_library_path(); - let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c.html") - .unwrap(); - let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected - ); - } - - #[test] - fn test_get_bytes_at_path_when_omit_html_expect_data() { - initialize(); - let test_library_path = get_test_library_path(); - let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c") - .unwrap(); - let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected - ); - } - - #[test] - fn test_get_bytes_at_path_when_omit_index_expect_data() { - initialize(); - let test_library_path = get_test_library_path(); - let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/d") - .unwrap(); - let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected - ); - } - - #[test] - fn test_get_bytes_at_path_when_invalid_repo_namespace_expect_error() { - initialize(); - let test_library_path = get_test_library_path(); - let actual = Repo::new(&test_library_path, "xxx", "law-html").unwrap_err(); - let expected = "failed to resolve path"; - assert_eq!(&format!("{}", actual)[..22], expected); - } - - #[test] - fn test_get_bytes_at_path_when_invalid_repo_name_expect_error() { - initialize(); - let test_library_path = get_test_library_path(); - let actual = Repo::new(&test_library_path, "test", "xxx").unwrap_err(); - let expected = "failed to resolve path"; - assert_eq!(&format!("{}", actual)[..22], expected); - } - - #[test] - fn test_get_bytes_at_path_when_invalid_path_expect_error() { - initialize(); - let test_library_path = get_test_library_path(); - let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/x") - .unwrap_err(); - let expected = "Doesn't exist"; - assert_eq!(format!("{}", actual), expected); - } -} diff --git a/src/utils/library.rs b/src/utils/library.rs index 8a18f7b..e12925f 100644 --- a/src/utils/library.rs +++ b/src/utils/library.rs @@ -18,68 +18,3 @@ pub fn find_library_path(path: &Path) -> anyhow::Result { abs_path.to_string_lossy() ))) } - -#[allow(clippy::unwrap_used)] -#[allow(clippy::string_slice)] -#[allow(clippy::indexing_slicing)] -#[cfg(test)] -mod test { - use crate::utils::library::find_library_path; - use std::env::current_exe; - use std::path::PathBuf; - - fn get_test_library_path() -> PathBuf { - let mut library_path = current_exe() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .parent() - .unwrap() - .to_owned(); - library_path.push("test"); - library_path.push("library"); - library_path.canonicalize().unwrap() - } - - #[test] - fn test_find_library_path_when_at_library_expect_path() { - let library_path = get_test_library_path(); - let actual = find_library_path(&library_path).unwrap(); - let expected = library_path; - assert_eq!(actual, expected); - } - - #[test] - fn test_find_library_path_when_in_library_expect_library_path() { - let library_path = get_test_library_path(); - let cwd = library_path.join("test"); - let actual = find_library_path(&cwd).unwrap(); - let expected = library_path; - assert_eq!(actual, expected); - } - - #[test] - fn test_find_library_path_when_nonexistant_path_expect_error() { - let library_path = get_test_library_path(); - let cwd = library_path.join("does_not_exist"); - let actual_err = find_library_path(&cwd).unwrap_err(); - let actual = format!("{}", actual_err); - let expected = "(os error 2)"; - assert_eq!(&actual[actual.len() - 12..], expected); - } - - #[test] - fn test_find_library_path_when_not_in_library_expect_error() { - let library_path = get_test_library_path(); - let cwd = library_path.parent().unwrap(); - let actual_err = find_library_path(cwd).unwrap_err(); - let actual = format!("{}", actual_err); - let expected = - "is not inside a Stele Library. Run `stele init` to create a library at this location."; - assert_eq!(&actual[actual.len() - 85..], expected); - } -} diff --git a/tests/basic/gitrepo_test.rs b/tests/basic/gitrepo_test.rs new file mode 100644 index 0000000..91c935c --- /dev/null +++ b/tests/basic/gitrepo_test.rs @@ -0,0 +1,93 @@ +use stele::utils::git::Repo; + +use crate::common; + +#[test] +fn test_get_bytes_at_path_when_empty_path_expect_index_html() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); + let actual = repo + .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "") + .unwrap(); + let expected = ""; + assert_eq!( + &core::str::from_utf8(actual.as_slice()).unwrap()[..15], + expected + ); +} + +#[test] +fn test_get_bytes_at_path_when_full_path_expect_data() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); + let actual = repo + .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c.html") + .unwrap(); + let expected = ""; + assert_eq!( + &std::str::from_utf8(actual.as_slice()).unwrap()[..15], + expected + ); +} + +#[test] +fn test_get_bytes_at_path_when_omit_html_expect_data() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); + let actual = repo + .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c") + .unwrap(); + let expected = ""; + assert_eq!( + &std::str::from_utf8(actual.as_slice()).unwrap()[..15], + expected + ); +} + +#[test] +fn test_get_bytes_at_path_when_omit_index_expect_data() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); + let actual = repo + .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/d") + .unwrap(); + let expected = ""; + assert_eq!( + &std::str::from_utf8(actual.as_slice()).unwrap()[..15], + expected + ); +} + +#[test] +fn test_get_bytes_at_path_when_invalid_repo_namespace_expect_error() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let actual = Repo::new(&test_library_path, "xxx", "law-html").unwrap_err(); + let expected = "failed to resolve path"; + assert_eq!(&format!("{}", actual)[..22], expected); +} + +#[test] +fn test_get_bytes_at_path_when_invalid_repo_name_expect_error() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let actual = Repo::new(&test_library_path, "test", "xxx").unwrap_err(); + let expected = "failed to resolve path"; + assert_eq!(&format!("{}", actual)[..22], expected); +} + +#[test] +fn test_get_bytes_at_path_when_invalid_path_expect_error() { + common::initialize(); + let test_library_path = common::get_test_library_path(); + let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); + let actual = repo + .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/x") + .unwrap_err(); + let expected = "Doesn't exist"; + assert_eq!(format!("{}", actual), expected); +} diff --git a/tests/basic/library_test.rs b/tests/basic/library_test.rs new file mode 100644 index 0000000..acd191d --- /dev/null +++ b/tests/basic/library_test.rs @@ -0,0 +1,41 @@ +use stele::utils::library::find_library_path; + +use crate::common; + +#[test] +fn test_find_library_path_when_at_library_expect_path() { + let library_path = common::get_test_library_path(); + let actual = find_library_path(&library_path).unwrap(); + let expected = library_path; + assert_eq!(actual, expected); +} + +#[test] +fn test_find_library_path_when_in_library_expect_library_path() { + let library_path = common::get_test_library_path(); + let cwd = library_path.join("test"); + let actual = find_library_path(&cwd).unwrap(); + let expected = library_path; + assert_eq!(actual, expected); +} + +#[test] +fn test_find_library_path_when_nonexistant_path_expect_error() { + let library_path = common::get_test_library_path(); + let cwd = library_path.join("does_not_exist"); + let actual_err = find_library_path(&cwd).unwrap_err(); + let actual = format!("{}", actual_err); + let expected = "(os error 2)"; + assert_eq!(&actual[actual.len() - 12..], expected); +} + +#[test] +fn test_find_library_path_when_not_in_library_expect_error() { + let library_path = common::get_test_library_path(); + let cwd = library_path.parent().unwrap(); + let actual_err = find_library_path(cwd).unwrap_err(); + let actual = format!("{}", actual_err); + let expected = + "is not inside a Stele Library. Run `stele init` to create a library at this location."; + assert_eq!(&actual[actual.len() - 85..], expected); +} diff --git a/tests/basic/mod.rs b/tests/basic/mod.rs new file mode 100644 index 0000000..f0aaa5c --- /dev/null +++ b/tests/basic/mod.rs @@ -0,0 +1,2 @@ +mod gitrepo_test; +mod library_test; diff --git a/tests/basic_test.rs b/tests/basic_test.rs new file mode 100644 index 0000000..7093860 --- /dev/null +++ b/tests/basic_test.rs @@ -0,0 +1,5 @@ +#![allow(clippy::pedantic)] +#![allow(clippy::restriction)] + +mod basic; +mod common; diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 0000000..0b7abb6 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,21 @@ +use std::fs::create_dir_all; +use std::path::PathBuf; +use std::sync::Once; + +static INIT: Once = Once::new(); + +pub fn initialize() { + INIT.call_once(|| { + let repo_path = get_test_library_path().join(PathBuf::from("test/law-html")); + let heads_path = repo_path.join(PathBuf::from("refs/heads")); + create_dir_all(heads_path).unwrap(); + let tags_path = repo_path.join(PathBuf::from("refs/tags")); + create_dir_all(tags_path).unwrap(); + }); +} + +pub fn get_test_library_path() -> PathBuf { + let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + path.push("tests/fixtures/library"); + path +} diff --git a/test/library/.stele/.keep b/tests/fixtures/library/.stele/.keep similarity index 100% rename from test/library/.stele/.keep rename to tests/fixtures/library/.stele/.keep diff --git a/test/library/test/law-html/HEAD b/tests/fixtures/library/test/law-html/HEAD similarity index 100% rename from test/library/test/law-html/HEAD rename to tests/fixtures/library/test/law-html/HEAD diff --git a/test/library/test/law-html/config b/tests/fixtures/library/test/law-html/config similarity index 100% rename from test/library/test/law-html/config rename to tests/fixtures/library/test/law-html/config diff --git a/test/library/test/law-html/description b/tests/fixtures/library/test/law-html/description similarity index 100% rename from test/library/test/law-html/description rename to tests/fixtures/library/test/law-html/description diff --git a/test/library/test/law-html/hooks/applypatch-msg.sample b/tests/fixtures/library/test/law-html/hooks/applypatch-msg.sample similarity index 100% rename from test/library/test/law-html/hooks/applypatch-msg.sample rename to tests/fixtures/library/test/law-html/hooks/applypatch-msg.sample diff --git a/test/library/test/law-html/hooks/commit-msg.sample b/tests/fixtures/library/test/law-html/hooks/commit-msg.sample similarity index 100% rename from test/library/test/law-html/hooks/commit-msg.sample rename to tests/fixtures/library/test/law-html/hooks/commit-msg.sample diff --git a/test/library/test/law-html/hooks/fsmonitor-watchman.sample b/tests/fixtures/library/test/law-html/hooks/fsmonitor-watchman.sample similarity index 100% rename from test/library/test/law-html/hooks/fsmonitor-watchman.sample rename to tests/fixtures/library/test/law-html/hooks/fsmonitor-watchman.sample diff --git a/test/library/test/law-html/hooks/post-update.sample b/tests/fixtures/library/test/law-html/hooks/post-update.sample similarity index 100% rename from test/library/test/law-html/hooks/post-update.sample rename to tests/fixtures/library/test/law-html/hooks/post-update.sample diff --git a/test/library/test/law-html/hooks/pre-applypatch.sample b/tests/fixtures/library/test/law-html/hooks/pre-applypatch.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-applypatch.sample rename to tests/fixtures/library/test/law-html/hooks/pre-applypatch.sample diff --git a/test/library/test/law-html/hooks/pre-commit.sample b/tests/fixtures/library/test/law-html/hooks/pre-commit.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-commit.sample rename to tests/fixtures/library/test/law-html/hooks/pre-commit.sample diff --git a/test/library/test/law-html/hooks/pre-merge-commit.sample b/tests/fixtures/library/test/law-html/hooks/pre-merge-commit.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-merge-commit.sample rename to tests/fixtures/library/test/law-html/hooks/pre-merge-commit.sample diff --git a/test/library/test/law-html/hooks/pre-push.sample b/tests/fixtures/library/test/law-html/hooks/pre-push.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-push.sample rename to tests/fixtures/library/test/law-html/hooks/pre-push.sample diff --git a/test/library/test/law-html/hooks/pre-rebase.sample b/tests/fixtures/library/test/law-html/hooks/pre-rebase.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-rebase.sample rename to tests/fixtures/library/test/law-html/hooks/pre-rebase.sample diff --git a/test/library/test/law-html/hooks/pre-receive.sample b/tests/fixtures/library/test/law-html/hooks/pre-receive.sample similarity index 100% rename from test/library/test/law-html/hooks/pre-receive.sample rename to tests/fixtures/library/test/law-html/hooks/pre-receive.sample diff --git a/test/library/test/law-html/hooks/prepare-commit-msg.sample b/tests/fixtures/library/test/law-html/hooks/prepare-commit-msg.sample similarity index 100% rename from test/library/test/law-html/hooks/prepare-commit-msg.sample rename to tests/fixtures/library/test/law-html/hooks/prepare-commit-msg.sample diff --git a/test/library/test/law-html/hooks/update.sample b/tests/fixtures/library/test/law-html/hooks/update.sample similarity index 100% rename from test/library/test/law-html/hooks/update.sample rename to tests/fixtures/library/test/law-html/hooks/update.sample diff --git a/test/library/test/law-html/info/exclude b/tests/fixtures/library/test/law-html/info/exclude similarity index 100% rename from test/library/test/law-html/info/exclude rename to tests/fixtures/library/test/law-html/info/exclude diff --git a/test/library/test/law-html/objects/2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c b/tests/fixtures/library/test/law-html/objects/2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c similarity index 100% rename from test/library/test/law-html/objects/2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c rename to tests/fixtures/library/test/law-html/objects/2c/cb5e590db7c6aba5166a600a9b18e9f9ec237c diff --git a/test/library/test/law-html/objects/b6/4fe7f7ea66c117596363e1179e30892a40572c b/tests/fixtures/library/test/law-html/objects/b6/4fe7f7ea66c117596363e1179e30892a40572c similarity index 100% rename from test/library/test/law-html/objects/b6/4fe7f7ea66c117596363e1179e30892a40572c rename to tests/fixtures/library/test/law-html/objects/b6/4fe7f7ea66c117596363e1179e30892a40572c diff --git a/test/library/test/law-html/objects/c5/3c5c71365e1d88752ed223b14f437d698c8806 b/tests/fixtures/library/test/law-html/objects/c5/3c5c71365e1d88752ed223b14f437d698c8806 similarity index 100% rename from test/library/test/law-html/objects/c5/3c5c71365e1d88752ed223b14f437d698c8806 rename to tests/fixtures/library/test/law-html/objects/c5/3c5c71365e1d88752ed223b14f437d698c8806 diff --git a/test/library/test/law-html/objects/info/commit-graph b/tests/fixtures/library/test/law-html/objects/info/commit-graph similarity index 100% rename from test/library/test/law-html/objects/info/commit-graph rename to tests/fixtures/library/test/law-html/objects/info/commit-graph diff --git a/test/library/test/law-html/objects/info/packs b/tests/fixtures/library/test/law-html/objects/info/packs similarity index 100% rename from test/library/test/law-html/objects/info/packs rename to tests/fixtures/library/test/law-html/objects/info/packs diff --git a/test/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx b/tests/fixtures/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx similarity index 100% rename from test/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx rename to tests/fixtures/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.idx diff --git a/test/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.pack b/tests/fixtures/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.pack similarity index 100% rename from test/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.pack rename to tests/fixtures/library/test/law-html/objects/pack/pack-0e5ff518cf56dc47a9481ab25de6b0fff831b5ce.pack diff --git a/test/library/test/law-html/packed-refs b/tests/fixtures/library/test/law-html/packed-refs similarity index 100% rename from test/library/test/law-html/packed-refs rename to tests/fixtures/library/test/law-html/packed-refs From faed2937b35957efe8e2b10e5bb4aa9a4bb16f74 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 13:34:04 -0300 Subject: [PATCH 06/15] chore: Good example of `Err` to `?` refactor This is a good example of exactly why `?` can be so powerful. `Err() => continue` or `Err() => return` are essentially synonyms for `?` --- src/utils/git.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/utils/git.rs b/src/utils/git.rs index 00108a9..2db0003 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -1,5 +1,6 @@ //! The git module contains structs for interacting with git repositories //! in the Stele Library. +use anyhow::Context; use git2::{Error, Repository}; use std::{fmt, path::Path}; @@ -35,7 +36,6 @@ impl Repo { /// /// Will return `Err` if git repository does not exist at `{namespace}/{name}` /// in library, or if there is something wrong with the git repository. - pub fn new(lib_path: &Path, namespace: &str, name: &str) -> Result { let lib_path_str = lib_path.to_string_lossy(); let repo_path = format!("{lib_path_str}/{namespace}/{name}"); @@ -64,20 +64,19 @@ impl Repo { pub fn get_bytes_at_path(&self, commitish: &str, path: &str) -> anyhow::Result> { let base_revision = format!("{commitish}:{path}"); for postfix in ["", "/index.html", ".html", "index.html"] { - match self - .repo - .revparse_single(&format!("{base_revision}{postfix}")) - { - Ok(obj) => { - let blob = match obj.into_blob() { - Ok(blob) => blob, - Err(_) => continue, - }; - return Ok(blob.content().to_owned()); - } - Err(_) => continue, + let query = &format!("{base_revision}{postfix}"); + let blob = self.find(query); + if blob.is_ok() { + return blob; } } Err(anyhow::anyhow!("Doesn't exist")) } + + /// Find something like `abc123:/path/to/something.txt` in the Git repo + fn find(&self, query: &str) -> anyhow::Result> { + let obj = self.repo.revparse_single(query)?; + let blob = obj.as_blob().context("Couldn't cast Git object to blob")?; + Ok(blob.content().to_owned()) + } } From 4f2fa1e2e382efb903f6c4689467a380861254e6 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 19:32:38 -0300 Subject: [PATCH 07/15] chore: Top-level function to match anyhow errors This demonstrates the "unpacking" of the potentially diverse automagically collected anyhow errors assocaited with finding and getting a Git blob. See `blob_error_response()`. Internal errors are safely matched to the appropriate user-facing HTTP responses. Note the use of the helpful `anyhow::bail!` macro that can be used in place of `Err(anyhow::anyhow!("error message..."))`. Also note how `async fn get_blob()` only needs to match for `Err` in just one place because of generous use of `?` in the refactored `fn find_blob()`. --- src/server/git.rs | 55 ++++++++++++++++++++++++++++--------- src/utils/git.rs | 9 ++++-- src/utils/library.rs | 4 +-- tests/basic/gitrepo_test.rs | 4 +-- 4 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/server/git.rs b/src/server/git.rs index 9095fcf..ad07dae 100644 --- a/src/server/git.rs +++ b/src/server/git.rs @@ -7,12 +7,13 @@ clippy::unused_async )] -use crate::utils::git::Repo; +use crate::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; use crate::utils::http::get_contenttype; use actix_web::{get, web, App, HttpResponse, HttpServer, Responder}; +use git2; use lazy_static::lazy_static; use regex::Regex; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// Global, read-only state passed into the actix app struct AppState { @@ -39,20 +40,48 @@ async fn get_blob( ) -> impl Responder { let (namespace, name, commitish, remainder) = path.into_inner(); let lib_path = &data.library_path; - let repo = match Repo::new(lib_path, &namespace, &name) { - Ok(repo) => repo, - Err(_e) => { - return HttpResponse::NotFound().body(format!("repo {namespace}/{name} does not exist")) - } - }; + let blob = find_blob(lib_path, &namespace, &name, &remainder, &commitish); let blob_path = clean_path(&remainder); let contenttype = get_contenttype(&blob_path); - - match repo.get_bytes_at_path(&commitish, &blob_path) { + match blob { Ok(content) => HttpResponse::Ok().insert_header(contenttype).body(content), - Err(_e) => HttpResponse::NotFound().body(format!( - "content at {remainder} for {commitish} in repo {namespace}/{name} does not exist" - )), + Err(error) => blob_error_response(&error, &namespace, &name), + } +} + +/// Do the work of looking for the requested Git object. +// TODO: This, and `clean_path`, look like they could live in `utils::git::Repo` +fn find_blob( + lib_path: &Path, + namespace: &str, + name: &str, + remainder: &str, + commitish: &str, +) -> anyhow::Result> { + let repo = Repo::new(lib_path, namespace, name)?; + let blob_path = clean_path(remainder); + let blob = repo.get_bytes_at_path(commitish, &blob_path)?; + Ok(blob) +} + +/// A centralised place to match potentially unsafe internal errors to safe user-facing error responses +#[allow(clippy::wildcard_enum_match_arm)] +fn blob_error_response(error: &anyhow::Error, namespace: &str, name: &str) -> HttpResponse { + if let Some(git_error) = error.downcast_ref::() { + return match git_error.code() { + // TODO: check this is the right error + git2::ErrorCode::NotFound => { + HttpResponse::NotFound().body(format!("repo {namespace}/{name} does not exist")) + } + _ => HttpResponse::InternalServerError().body("Unexpected Git error"), + }; + } + match error { + // TODO: Obviously it's better to use custom `Error` types + _ if error.to_string() == GIT_REQUEST_NOT_FOUND => { + HttpResponse::NotFound().body(GIT_REQUEST_NOT_FOUND) + } + _ => HttpResponse::InternalServerError().body("Unexpected server error"), } } diff --git a/src/utils/git.rs b/src/utils/git.rs index 2db0003..7169cac 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -1,9 +1,12 @@ //! The git module contains structs for interacting with git repositories //! in the Stele Library. use anyhow::Context; -use git2::{Error, Repository}; +use git2::Repository; use std::{fmt, path::Path}; +/// This is the first step towards having custom errors +pub const GIT_REQUEST_NOT_FOUND: &str = "Git object doesn't exist"; + /// Represents a git repository within an oll library. includes helpers for /// for interacting with the Git Repo. /// Expects a path to the library, as well as the repo's namespace and name. @@ -36,7 +39,7 @@ impl Repo { /// /// Will return `Err` if git repository does not exist at `{namespace}/{name}` /// in library, or if there is something wrong with the git repository. - pub fn new(lib_path: &Path, namespace: &str, name: &str) -> Result { + pub fn new(lib_path: &Path, namespace: &str, name: &str) -> anyhow::Result { let lib_path_str = lib_path.to_string_lossy(); let repo_path = format!("{lib_path_str}/{namespace}/{name}"); Ok(Self { @@ -70,7 +73,7 @@ impl Repo { return blob; } } - Err(anyhow::anyhow!("Doesn't exist")) + anyhow::bail!(GIT_REQUEST_NOT_FOUND) } /// Find something like `abc123:/path/to/something.txt` in the Git repo diff --git a/src/utils/library.rs b/src/utils/library.rs index e12925f..ce2212d 100644 --- a/src/utils/library.rs +++ b/src/utils/library.rs @@ -13,8 +13,8 @@ pub fn find_library_path(path: &Path) -> anyhow::Result { return Ok(working_path.to_owned()); } } - Err(anyhow::anyhow!(format!( + anyhow::bail!(format!( "{} is not inside a Stele Library. Run `stele init` to create a library at this location.", abs_path.to_string_lossy() - ))) + )) } diff --git a/tests/basic/gitrepo_test.rs b/tests/basic/gitrepo_test.rs index 91c935c..5655d89 100644 --- a/tests/basic/gitrepo_test.rs +++ b/tests/basic/gitrepo_test.rs @@ -1,4 +1,4 @@ -use stele::utils::git::Repo; +use stele::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; use crate::common; @@ -88,6 +88,6 @@ fn test_get_bytes_at_path_when_invalid_path_expect_error() { let actual = repo .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/x") .unwrap_err(); - let expected = "Doesn't exist"; + let expected = GIT_REQUEST_NOT_FOUND; assert_eq!(format!("{}", actual), expected); } From 2908d663a01e55324f97ebe10c287b50e417b89d Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 21:53:27 -0300 Subject: [PATCH 08/15] chore: Use `Into` trait for known and basic types Often you can just lean on Rust's type system and built-in typecasting functionality. See: https://doc.rust-lang.org/rust-by-example/conversion/from_into.html --- src/utils/git.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils/git.rs b/src/utils/git.rs index 7169cac..41c8d97 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -43,9 +43,9 @@ impl Repo { let lib_path_str = lib_path.to_string_lossy(); let repo_path = format!("{lib_path_str}/{namespace}/{name}"); Ok(Self { - lib_path: String::from(lib_path_str), - namespace: String::from(namespace), - name: String::from(name), + lib_path: lib_path_str.into(), + namespace: namespace.into(), + name: name.into(), repo: Repository::open(repo_path)?, }) } From 1aea99a2881d736e8f6d0ad9ac280ad5f0307fe2 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 8 Dec 2022 22:19:32 -0300 Subject: [PATCH 09/15] chore: Use `String` matchers rather than array[] Generally speaking I think array[] access is only used in very perofrmance critical applications. --- tests/basic/gitrepo_test.rs | 65 ++++++++++++++++++++----------------- tests/basic/library_test.rs | 16 +++++---- 2 files changed, 45 insertions(+), 36 deletions(-) diff --git a/tests/basic/gitrepo_test.rs b/tests/basic/gitrepo_test.rs index 5655d89..0cec13d 100644 --- a/tests/basic/gitrepo_test.rs +++ b/tests/basic/gitrepo_test.rs @@ -2,18 +2,22 @@ use stele::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; use crate::common; +const COMMIT: &str = "ed782e08d119a580baa3067e2ea5df06f3d1cd05"; + +fn blob_to_string(blob: Vec) -> String { + core::str::from_utf8(blob.as_slice()).unwrap().into() +} + #[test] fn test_get_bytes_at_path_when_empty_path_expect_index_html() { common::initialize(); let test_library_path = common::get_test_library_path(); let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "") - .unwrap(); + let actual = repo.get_bytes_at_path(COMMIT, "").unwrap(); let expected = ""; - assert_eq!( - &core::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected + assert!( + blob_to_string(actual).starts_with(expected), + "doesn't start with {expected}" ); } @@ -22,13 +26,11 @@ fn test_get_bytes_at_path_when_full_path_expect_data() { common::initialize(); let test_library_path = common::get_test_library_path(); let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c.html") - .unwrap(); + let actual = repo.get_bytes_at_path(COMMIT, "a/b/c.html").unwrap(); let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected + assert!( + blob_to_string(actual).starts_with(expected), + "doesn't start with {expected}" ); } @@ -37,13 +39,11 @@ fn test_get_bytes_at_path_when_omit_html_expect_data() { common::initialize(); let test_library_path = common::get_test_library_path(); let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/c") - .unwrap(); + let actual = repo.get_bytes_at_path(COMMIT, "a/b/c").unwrap(); let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected + assert!( + blob_to_string(actual).starts_with(expected), + "doesn't start with {expected}" ); } @@ -52,13 +52,11 @@ fn test_get_bytes_at_path_when_omit_index_expect_data() { common::initialize(); let test_library_path = common::get_test_library_path(); let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/d") - .unwrap(); + let actual = repo.get_bytes_at_path(COMMIT, "a/b/d").unwrap(); let expected = ""; - assert_eq!( - &std::str::from_utf8(actual.as_slice()).unwrap()[..15], - expected + assert!( + blob_to_string(actual).starts_with(expected), + "doesn't start with {expected}" ); } @@ -68,7 +66,10 @@ fn test_get_bytes_at_path_when_invalid_repo_namespace_expect_error() { let test_library_path = common::get_test_library_path(); let actual = Repo::new(&test_library_path, "xxx", "law-html").unwrap_err(); let expected = "failed to resolve path"; - assert_eq!(&format!("{}", actual)[..22], expected); + assert!( + actual.to_string().contains(expected), + "\"{actual}\" doesn't contain {expected}" + ); } #[test] @@ -77,7 +78,10 @@ fn test_get_bytes_at_path_when_invalid_repo_name_expect_error() { let test_library_path = common::get_test_library_path(); let actual = Repo::new(&test_library_path, "test", "xxx").unwrap_err(); let expected = "failed to resolve path"; - assert_eq!(&format!("{}", actual)[..22], expected); + assert!( + actual.to_string().contains(expected), + "\"{actual}\" doesn't contain {expected}" + ); } #[test] @@ -85,9 +89,10 @@ fn test_get_bytes_at_path_when_invalid_path_expect_error() { common::initialize(); let test_library_path = common::get_test_library_path(); let repo = Repo::new(&test_library_path, "test", "law-html").unwrap(); - let actual = repo - .get_bytes_at_path("ed782e08d119a580baa3067e2ea5df06f3d1cd05", "a/b/x") - .unwrap_err(); + let actual = repo.get_bytes_at_path(COMMIT, "a/b/x").unwrap_err(); let expected = GIT_REQUEST_NOT_FOUND; - assert_eq!(format!("{}", actual), expected); + assert!( + actual.to_string().contains(expected), + "\"{actual}\" doesn't contain {expected}" + ); } diff --git a/tests/basic/library_test.rs b/tests/basic/library_test.rs index acd191d..bc0b0d9 100644 --- a/tests/basic/library_test.rs +++ b/tests/basic/library_test.rs @@ -23,19 +23,23 @@ fn test_find_library_path_when_in_library_expect_library_path() { fn test_find_library_path_when_nonexistant_path_expect_error() { let library_path = common::get_test_library_path(); let cwd = library_path.join("does_not_exist"); - let actual_err = find_library_path(&cwd).unwrap_err(); - let actual = format!("{}", actual_err); + let actual = find_library_path(&cwd).unwrap_err(); let expected = "(os error 2)"; - assert_eq!(&actual[actual.len() - 12..], expected); + assert!( + actual.to_string().contains(expected), + "\"{actual}\" doesn't contain {expected}" + ); } #[test] fn test_find_library_path_when_not_in_library_expect_error() { let library_path = common::get_test_library_path(); let cwd = library_path.parent().unwrap(); - let actual_err = find_library_path(cwd).unwrap_err(); - let actual = format!("{}", actual_err); + let actual = find_library_path(cwd).unwrap_err(); let expected = "is not inside a Stele Library. Run `stele init` to create a library at this location."; - assert_eq!(&actual[actual.len() - 85..], expected); + assert!( + actual.to_string().contains(expected), + "\"{actual}\" doesn't contain {expected}" + ); } From 1ef0c90e1f3146cbefafac90571ba4b9d2b1c28d Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Fri, 9 Dec 2022 15:48:24 -0300 Subject: [PATCH 10/15] feat: Basic tracing/logging --- Cargo.lock | 80 +++++++++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 2 ++ README.md | 7 ++++- src/server/git.rs | 10 +++--- src/utils/cli.rs | 14 +++++++-- src/utils/git.rs | 3 ++ 6 files changed, 107 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9aba26d..97cdef2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1032,6 +1032,16 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "nu-ansi-term" +version = "0.46.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77a8165726e8236064dbb45459242600304b42a5ea24ee2948e18e023bf7ba84" +dependencies = [ + "overload", + "winapi", +] + [[package]] name = "num-traits" version = "0.2.15" @@ -1088,6 +1098,12 @@ version = "6.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b7820b9daea5457c9f21c69448905d723fbd21136ccf521748f23fd49e723ee" +[[package]] +name = "overload" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" + [[package]] name = "parking_lot" version = "0.12.1" @@ -1413,6 +1429,15 @@ dependencies = [ "digest", ] +[[package]] +name = "sharded-slab" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "900fba806f70c630b0a382d0d825e17a0f19fcd059a2ade1ff237bcddf446b31" +dependencies = [ + "lazy_static", +] + [[package]] name = "signal-hook-registry" version = "1.4.0" @@ -1460,6 +1485,8 @@ dependencies = [ "lazy_static", "regex", "serde", + "tracing", + "tracing-subscriber", ] [[package]] @@ -1497,6 +1524,15 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "thread_local" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5516c27b78311c50bf42c071425c560ac799b11c30b31f87e3081965fe5e0180" +dependencies = [ + "once_cell", +] + [[package]] name = "time" version = "0.3.17" @@ -1590,9 +1626,21 @@ dependencies = [ "cfg-if", "log", "pin-project-lite", + "tracing-attributes", "tracing-core", ] +[[package]] +name = "tracing-attributes" +version = "0.1.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4017f8f45139870ca7e672686113917c71c7a6e02d4924eda67186083c03081a" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "tracing-core" version = "0.1.30" @@ -1600,6 +1648,32 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24eb03ba0eab1fd845050058ce5e616558e8f8d8fca633e6b163fe25c797213a" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78ddad33d2d10b1ed7eb9d1f518a5674713876e97e5bb9b7345a7984fbb4f922" +dependencies = [ + "lazy_static", + "log", + "tracing-core", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a6176eae26dd70d0c919749377897b54a9276bd7061339665dd68777926b5a70" +dependencies = [ + "nu-ansi-term", + "sharded-slab", + "smallvec", + "thread_local", + "tracing-core", + "tracing-log", ] [[package]] @@ -1655,6 +1729,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "valuable" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" + [[package]] name = "vcpkg" version = "0.2.15" diff --git a/Cargo.toml b/Cargo.toml index 334f1d2..9404559 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,8 @@ git2 = "0.15" lazy_static = "1.4.0" regex = "1" serde = "1.0" +tracing = "0.1.37" +tracing-subscriber = "0.3.16" [dev-dependencies] criterion = "0.3" diff --git a/README.md b/README.md index d9d20b6..96816ef 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,6 @@ Stele is a system for distributing, preserving, and authenticating laws. 6. We recommend using [VSCode](https://code.visualstudio.com/Download) (default settings provided in repo), but you can use any editor you like. ### Development - - Lints must pass before merging into master - All code must have tests. Tests should conform to our testing guidelines. - Run `just` from within the repository to list all available just commands. Currently: @@ -31,6 +30,12 @@ Stele is a system for distributing, preserving, and authenticating laws. - `test`: Run all tests - On windows, especially, you may wish to run just through the nu shell, which can be done by calling all commands with the `--shell` command, e.g. `just --shell nu lint`. +## Logging + +The ENV variable `RUST_LOG` can be set with one of `trace`, `debug`, `info`, `warn`, `error`. Filters can be set based on the `target` components seen in the logs lines, for example: to use `trace` but turn down the noise from the Actix dispatcher: `RUST_LOG="trace,actix_http::h1::dispatcher=warn"` + +See [tracing-subscriber docs](https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/index.html#filtering-events-with-environment-variables) and [env_logger syntax](https://docs.rs/env_logger/latest/env_logger/#enabling-logging]). + ## Q&A - Why do we suggest NuShell? - NuShell is almost as fast on windows as cmd, but is compattible with bash. If you do not use NuShell on windows, you will need to make sure Git Bash is installed. If you have performance issues, consider switching to Nu. diff --git a/src/server/git.rs b/src/server/git.rs index ad07dae..8b40fed 100644 --- a/src/server/git.rs +++ b/src/server/git.rs @@ -87,16 +87,14 @@ fn blob_error_response(error: &anyhow::Error, namespace: &str, name: &str) -> Ht /// Serve git repositories in the Stele library. #[actix_web::main] // or #[tokio::main] -#[allow(clippy::print_stdout)] pub async fn serve_git( raw_library_path: &str, library_path: PathBuf, port: u16, ) -> std::io::Result<()> { - println!( - "Serving content from the Stele library at {} on http://127.0.0.1:{}.", - raw_library_path, port - ); + let bind = "127.0.0.1"; + let message = "Serving content from the Stele library at"; + tracing::info!("{message} '{raw_library_path}' on http://{bind}:{port}.",); HttpServer::new(move || { App::new() @@ -105,7 +103,7 @@ pub async fn serve_git( library_path: library_path.clone(), })) }) - .bind(("127.0.0.1", port))? + .bind((bind, port))? .run() .await } diff --git a/src/utils/cli.rs b/src/utils/cli.rs index 7584c7c..ec19d0f 100644 --- a/src/utils/cli.rs +++ b/src/utils/cli.rs @@ -7,6 +7,7 @@ use crate::server::git::serve_git; use crate::utils::library::find_library_path; use clap::Parser; use std::path::Path; +use tracing; /// Stele is currently just a simple git server. /// run from the library directory or pass @@ -33,18 +34,27 @@ enum Subcommands { }, } +/// +fn init_tracing() { + tracing_subscriber::fmt::init(); + if std::env::var("RUST_LOG").is_err() { + std::env::set_var("RUST_LOG", "info"); + } +} + /// Main entrypoint to application /// /// # Errors /// TODO: This function should not return errors -#[allow(clippy::print_stdout)] pub fn run() -> std::io::Result<()> { + init_tracing(); + tracing::debug!("Starting application"); let cli = Cli::parse(); let library_path_wd = Path::new(&cli.library_path); let library_path = if let Ok(lpath) = find_library_path(library_path_wd) { lpath } else { - println!( + tracing::error!( "error: could not find `.stele` folder in `{}` or any parent directory", &cli.library_path ); diff --git a/src/utils/git.rs b/src/utils/git.rs index 41c8d97..1ec84e6 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -70,14 +70,17 @@ impl Repo { let query = &format!("{base_revision}{postfix}"); let blob = self.find(query); if blob.is_ok() { + tracing::trace!(query, "Found Git object"); return blob; } } + tracing::debug!(base_revision, "Couldn't find requeted Git object"); anyhow::bail!(GIT_REQUEST_NOT_FOUND) } /// Find something like `abc123:/path/to/something.txt` in the Git repo fn find(&self, query: &str) -> anyhow::Result> { + tracing::trace!(query, "Git reverse parse search"); let obj = self.repo.revparse_single(query)?; let blob = obj.as_blob().context("Couldn't cast Git object to blob")?; Ok(blob.content().to_owned()) From e216ad8dc3759309d43c9e0cc522aca4440593fa Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Fri, 9 Dec 2022 20:11:12 -0300 Subject: [PATCH 11/15] feat: Basic structure for custom request logging All HTTP requests are now logged with useful data. And other traces, debugs, infos, etc, that are logged during the lifetime of a HTTP request are also prepened with the that same HTTP context data. Also an example of adding custom request data and conditionally acting on that data is included. Namely, adding request duration and logging a warning when the duration is over a certain amount of time. --- Cargo.lock | 42 ++++++++++++++++++++++++ Cargo.toml | 1 + src/lib.rs | 5 ++- src/server.rs | 1 + src/server/git.rs | 5 +-- src/server/tracing.rs | 76 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 src/server/tracing.rs diff --git a/Cargo.lock b/Cargo.lock index 97cdef2..742bedf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1139,6 +1139,26 @@ version = "2.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "478c572c3d73181ff3c2539045f6eb99e5491218eae919370993b890cdbdd98e" +[[package]] +name = "pin-project" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad29a609b6bcd67fee905812e544992d216af9d755757c05ed2d0e15a74c6ecc" +dependencies = [ + "pin-project-internal", +] + +[[package]] +name = "pin-project-internal" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "069bdb1e05adc7a8990dce9cc75370895fbe4e3d58b9b73bf1aee56359344a55" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "pin-project-lite" version = "0.2.9" @@ -1486,6 +1506,7 @@ dependencies = [ "regex", "serde", "tracing", + "tracing-actix-web", "tracing-subscriber", ] @@ -1630,6 +1651,18 @@ dependencies = [ "tracing-core", ] +[[package]] +name = "tracing-actix-web" +version = "0.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d725b8fa6ef307b3f4856913523337de45c47cc79271bafd7acfb39559e3a2da" +dependencies = [ + "actix-web", + "pin-project", + "tracing", + "uuid", +] + [[package]] name = "tracing-attributes" version = "0.1.23" @@ -1729,6 +1762,15 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "uuid" +version = "1.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "422ee0de9031b5b948b97a8fc04e3aa35230001a722ddd27943e0be31564ce4c" +dependencies = [ + "getrandom", +] + [[package]] name = "valuable" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 9404559..db1308e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ regex = "1" serde = "1.0" tracing = "0.1.37" tracing-subscriber = "0.3.16" +tracing-actix-web = "0.6.2" [dev-dependencies] criterion = "0.3" diff --git a/src/lib.rs b/src/lib.rs index 8c43329..572bd1a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -76,7 +76,10 @@ // `#[inline]` doesn't mean that a function won't be inlined. And if performance does start // to become a problem, there are other avenues to explore before deciding on which functions // would benefit from explicit inlining. - clippy::missing_inline_in_public_items + clippy::missing_inline_in_public_items, + + // I think marking `#[non_exhaustive]` is more for structs that are imported into other crates + clippy::exhaustive_structs )] pub mod server; diff --git a/src/server.rs b/src/server.rs index a384013..a191d65 100644 --- a/src/server.rs +++ b/src/server.rs @@ -3,3 +3,4 @@ //! Currently contains only a git microserver. pub mod git; +pub mod tracing; diff --git a/src/server/git.rs b/src/server/git.rs index 8b40fed..1133bad 100644 --- a/src/server/git.rs +++ b/src/server/git.rs @@ -1,12 +1,11 @@ //! Legacy git microserver. #![allow( - // Will be deprecated upon completion of Publish Server migration to rust. - clippy::exhaustive_structs, // Unused asyncs are the norm in Actix route definition files clippy::unused_async )] +use crate::server::tracing::SteleRootSpanBuilder; use crate::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; use crate::utils::http::get_contenttype; use actix_web::{get, web, App, HttpResponse, HttpServer, Responder}; @@ -14,6 +13,7 @@ use git2; use lazy_static::lazy_static; use regex::Regex; use std::path::{Path, PathBuf}; +use tracing_actix_web::TracingLogger; /// Global, read-only state passed into the actix app struct AppState { @@ -98,6 +98,7 @@ pub async fn serve_git( HttpServer::new(move || { App::new() + .wrap(TracingLogger::::new()) .service(get_blob) .app_data(web::Data::new(AppState { library_path: library_path.clone(), diff --git a/src/server/tracing.rs b/src/server/tracing.rs new file mode 100644 index 0000000..5847d7a --- /dev/null +++ b/src/server/tracing.rs @@ -0,0 +1,76 @@ +//! Tracing/logging for HTTP servers + +use std::time::Instant; + +use actix_web::{ + dev::{ServiceRequest, ServiceResponse}, + HttpMessage, +}; +use tracing_actix_web::{DefaultRootSpanBuilder, RootSpanBuilder}; + +/// The length of time in milliseconds after which a request is considered slow +const SLOW_REQUEST_MS: u128 = 5 * 1000; + +/// More or less an alias just to add custom functionality to `DefaultRootSpanBuilder` +pub struct SteleRootSpanBuilder; + +/// For measuring the duration of a request +struct RequestStart(Instant); + +impl RootSpanBuilder for SteleRootSpanBuilder { + fn on_request_start(request: &ServiceRequest) -> tracing::Span { + // The `{}` block to tells the compiler to return ownership of `request`. + // NOTE: + // Because the `request` variable is included in the `*span!` macro, we're not likely to + // get linting feedback that the macro also mutably borrows `request`. Or at least I + // think that's why I only discovered the second mutable borrow _after_ running the app. + // Quite unusual for Rust to not pick up on it at compile time. + { + let mut request_extensions = request.extensions_mut(); + request_extensions.insert(RequestStart(Instant::now())); + } + + // The `RootSpan` is the data that is included with every `tracing::*` call during the + // lifetime of a HTTP request. It contains things like the user agent, HTTP path, etc. + // It can get quite noisy when tracing lots of non HTTP-related activity. But that is + // likely the fair price to pay for being able to sanely associate the log line with + // the request. Recall that in production there are likely to be many simultaneous requests + // making it hard to "read" the journey of a single request. A unique `request_id` is also + // included, so it would certainly be possible to disable the verbose default data, and + // then manually match the HTTP request's `request_id` with other log lines' `request_id`s. + tracing_actix_web::root_span!( + request, + duration_ms = tracing::field::Empty, + duration_ns = tracing::field::Empty, + ) + } + + fn on_request_end( + span: tracing::Span, + outcome: &Result, actix_web::Error>, + ) { + // TODO: + // I couldn't find a way of triggering the case where `outcome` is + // `Result::Err(actix_web::Error)`. It doesn't seem to be when a route method returns an + // error, as I assume that's considered a handled error. So maybe `outcome` is only ever + // and error for an Actix-internal error? Either way, the root span and timings all work + // normally for known and handled request errors. + outcome.as_ref().map_or((), |response| { + if let Some(req_start) = response.request().extensions().get::() { + let elapsed = req_start.0.elapsed(); + let millis = elapsed.as_millis(); + // Add the timings to the default `RootSpan` + span.record("duration_ms", millis); + span.record("duration_ns", elapsed.as_nanos()); + if millis > SLOW_REQUEST_MS { + tracing::warn!(duration_ms = millis, "Slow HTTP request"); + } else { + tracing::trace!("HTTP Request"); + } + } + }); + + // Captures the standard `RootSpan` fields + DefaultRootSpanBuilder::on_request_end(span, outcome); + } +} From 8a7bc00aff57a7cc6f95b316703b57335980478f Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Fri, 9 Dec 2022 20:48:09 -0300 Subject: [PATCH 12/15] feat: First custom Stele errors I needed this to manually test the tracing. So I tidied it up and made it the basis for more formally handling Stele-specific errors. --- Cargo.lock | 1 + Cargo.toml | 1 + src/lib.rs | 5 +++-- src/server.rs | 1 + src/server/errors.rs | 30 ++++++++++++++++++++++++++++++ src/server/git.rs | 25 ++++++++++++++++++++++--- src/server/tracing.rs | 4 ++-- 7 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 src/server/errors.rs diff --git a/Cargo.lock b/Cargo.lock index 742bedf..ef37736 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1501,6 +1501,7 @@ dependencies = [ "anyhow", "clap 4.0.27", "criterion", + "derive_more", "git2", "lazy_static", "regex", diff --git a/Cargo.toml b/Cargo.toml index db1308e..f87f831 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ serde = "1.0" tracing = "0.1.37" tracing-subscriber = "0.3.16" tracing-actix-web = "0.6.2" +derive_more = "0.99.17" [dev-dependencies] criterion = "0.3" diff --git a/src/lib.rs b/src/lib.rs index 572bd1a..49e2de9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -78,8 +78,9 @@ // would benefit from explicit inlining. clippy::missing_inline_in_public_items, - // I think marking `#[non_exhaustive]` is more for structs that are imported into other crates - clippy::exhaustive_structs + // I think marking `#[non_exhaustive]` is more for structs/enums that are imported into other crates + clippy::exhaustive_structs, + clippy::exhaustive_enums )] pub mod server; diff --git a/src/server.rs b/src/server.rs index a191d65..4ae2d58 100644 --- a/src/server.rs +++ b/src/server.rs @@ -2,5 +2,6 @@ //! //! Currently contains only a git microserver. +pub mod errors; pub mod git; pub mod tracing; diff --git a/src/server/errors.rs b/src/server/errors.rs new file mode 100644 index 0000000..a8f1d30 --- /dev/null +++ b/src/server/errors.rs @@ -0,0 +1,30 @@ +#![allow( + // derive_more doesn't respect these lints + clippy::pattern_type_mismatch, + clippy::use_self +)] + +//! Stele-specific errors + +use actix_web::{error, http::StatusCode, HttpResponse}; +use derive_more::{Display, Error}; + +/// Collection of possible Stele errors +#[derive(Debug, Display, Error)] +pub enum SteleError { + /// Errors generated by the Git server + #[display(fmt = "A Git server occurred")] + GitError, +} + +impl error::ResponseError for SteleError { + fn error_response(&self) -> HttpResponse { + HttpResponse::build(self.status_code()).body(self.to_string()) + } + + fn status_code(&self) -> StatusCode { + match *self { + Self::GitError => StatusCode::INTERNAL_SERVER_ERROR, + } + } +} diff --git a/src/server/git.rs b/src/server/git.rs index 1133bad..e3d9a1b 100644 --- a/src/server/git.rs +++ b/src/server/git.rs @@ -5,9 +5,6 @@ clippy::unused_async )] -use crate::server::tracing::SteleRootSpanBuilder; -use crate::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; -use crate::utils::http::get_contenttype; use actix_web::{get, web, App, HttpResponse, HttpServer, Responder}; use git2; use lazy_static::lazy_static; @@ -15,6 +12,11 @@ use regex::Regex; use std::path::{Path, PathBuf}; use tracing_actix_web::TracingLogger; +use super::errors::SteleError; +use crate::server::tracing::SteleRootSpanBuilder; +use crate::utils::git::{Repo, GIT_REQUEST_NOT_FOUND}; +use crate::utils::http::get_contenttype; + /// Global, read-only state passed into the actix app struct AppState { /// path to the Stele library @@ -30,6 +32,21 @@ fn clean_path(path: &str) -> String { RE.replace_all(path, "").to_string() } +/// Root index path +#[get("/")] +async fn index() -> &'static str { + "Welcome to Stele" +} + +/// Just for development purposes at the moment +#[get("{path}")] +async fn misc(path: web::Path) -> actix_web::Result<&'static str, SteleError> { + match path.as_str() { + "error" => Err(SteleError::GitError), + _ => Ok("\u{2728}"), + } +} + /// Return the content in the stele library in the `{namespace}/{name}` /// repo at the `commitish` commit at the `remainder` path. /// Return 404 if any are not found or there are any errors. @@ -99,6 +116,8 @@ pub async fn serve_git( HttpServer::new(move || { App::new() .wrap(TracingLogger::::new()) + .service(index) + .service(misc) .service(get_blob) .app_data(web::Data::new(AppState { library_path: library_path.clone(), diff --git a/src/server/tracing.rs b/src/server/tracing.rs index 5847d7a..709f27b 100644 --- a/src/server/tracing.rs +++ b/src/server/tracing.rs @@ -19,7 +19,7 @@ struct RequestStart(Instant); impl RootSpanBuilder for SteleRootSpanBuilder { fn on_request_start(request: &ServiceRequest) -> tracing::Span { - // The `{}` block to tells the compiler to return ownership of `request`. + // The `{}` block tells the compiler to return ownership of `request`. // NOTE: // Because the `request` variable is included in the `*span!` macro, we're not likely to // get linting feedback that the macro also mutably borrows `request`. Or at least I @@ -53,7 +53,7 @@ impl RootSpanBuilder for SteleRootSpanBuilder { // I couldn't find a way of triggering the case where `outcome` is // `Result::Err(actix_web::Error)`. It doesn't seem to be when a route method returns an // error, as I assume that's considered a handled error. So maybe `outcome` is only ever - // and error for an Actix-internal error? Either way, the root span and timings all work + // an error for an Actix-internal error? Either way, the root span and timings all work // normally for known and handled request errors. outcome.as_ref().map_or((), |response| { if let Some(req_start) = response.request().extensions().get::() { From 8c3e3241efdf64d61fb3cf607820906f0e970718 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Mon, 19 Dec 2022 18:38:06 -0300 Subject: [PATCH 13/15] fix: Rust min v1.66. Allow missing_trait_methods Hopefully fixes missing trait lint error --- Cargo.toml | 1 + src/server/errors.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index f87f831..d7eb72d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ license = "AGPL-3.0" keywords = ["authentication", "laws", "preservation"] categories = ["authentication", "web-programming::http-server"] repository = "https://github.com/openlawlibrary/stele" +rust-version = "1.66" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] diff --git a/src/server/errors.rs b/src/server/errors.rs index a8f1d30..8aabdfe 100644 --- a/src/server/errors.rs +++ b/src/server/errors.rs @@ -17,6 +17,7 @@ pub enum SteleError { GitError, } +#[allow(clippy::missing_trait_methods)] impl error::ResponseError for SteleError { fn error_response(&self) -> HttpResponse { HttpResponse::build(self.status_code()).body(self.to_string()) From 4ea9e4e8c6a49ae6f29a3fd7007674044b21cd6b Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Wed, 21 Dec 2022 19:42:02 -0300 Subject: [PATCH 14/15] chore: Use mod.rs instead slef-named modules --- src/lib.rs | 2 +- src/{server.rs => server/mod.rs} | 0 src/{utils.rs => utils/mod.rs} | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename src/{server.rs => server/mod.rs} (100%) rename src/{utils.rs => utils/mod.rs} (100%) diff --git a/src/lib.rs b/src/lib.rs index 49e2de9..01c9641 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -69,7 +69,7 @@ clippy::std_instead_of_core, // TODO: But I think the mod.rs is more conventional — @tombh - clippy::self_named_module_files, + clippy::mod_module_files, // Although performance is of course important for this application, it is not currently // such that it would benefit from explicit inline suggestions. Besides, not specifying diff --git a/src/server.rs b/src/server/mod.rs similarity index 100% rename from src/server.rs rename to src/server/mod.rs diff --git a/src/utils.rs b/src/utils/mod.rs similarity index 100% rename from src/utils.rs rename to src/utils/mod.rs From e2ebde838cd07b95dfed1ab730f0b766fd851a08 Mon Sep 17 00:00:00 2001 From: Thomas Buckley-Houston Date: Thu, 22 Dec 2022 23:01:13 -0300 Subject: [PATCH 15/15] chore: Remove etc/notes.md --- etc/NOTES.md | 29 ----------------------------- 1 file changed, 29 deletions(-) delete mode 100644 etc/NOTES.md diff --git a/etc/NOTES.md b/etc/NOTES.md deleted file mode 100644 index f3d1520..0000000 --- a/etc/NOTES.md +++ /dev/null @@ -1,29 +0,0 @@ -# Project Notes - -## Pedantic Rust Editor Settings -If you would like to make your editor always give you the most verbose feedback in any Rust project, you can use something like this. It can be used as-is in VSCode, or converted to something similar in your editor of choice. - -```json -{ - "rust-analyzer.checkOnSave.overrideCommand": [ - "cargo", - "clippy", - "--message-format=json", - "--all-targets", - "--all-features", - "--", - "-W", - "clippy::all", - "-W", - "clippy::pedantic", - "-W", - "clippy::restriction", - "-W", - "clippy::nursery", - "-W", - "clippy::cargo", - "-W", - "missing_docs" - ], -} -```