From 9a068bc67750301fab7e136440199824a92f9fc2 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Fri, 4 Aug 2017 11:53:15 +1000 Subject: [PATCH 1/5] Add more documentation Added documentation comments to krate::index and krate::new function and functions are called from these functions. Also added comments within the functions and on some API endpoints. --- src/git.rs | 6 ++++++ src/krate.rs | 31 +++++++++++++++++++++++++++++++ src/lib.rs | 3 +++ src/upload.rs | 4 ++++ src/version.rs | 9 +++++++++ 5 files changed, 53 insertions(+) diff --git a/src/git.rs b/src/git.rs index d2294cc1e27..a7489aacb90 100644 --- a/src/git.rs +++ b/src/git.rs @@ -73,6 +73,12 @@ pub fn add_crate(app: &App, krate: &Crate) -> CargoResult<()> { }) } +/// Yanks or unyanks a crate version. This requires finding the index +/// file, deserlialise the crate from JSON, change the yank boolean to +/// `true` or `false`, write all the lines back out, and commit and +/// push the changes. +// TODO: factor out the tasks this function does into smaller, separate +// functions. pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> CargoResult<()> { let repo = app.git_repo.lock().unwrap(); let repo_path = repo.workdir().unwrap(); diff --git a/src/krate.rs b/src/krate.rs index c8926d0867c..9dbd880b676 100644 --- a/src/krate.rs +++ b/src/krate.rs @@ -785,6 +785,26 @@ impl Model for Crate { } /// Handles the `GET /crates` route. +/// Returns a list of crates. Called in a variety of scenarios in the +/// front end, including: +/// - Alphabetical listing of crates +/// - List of crates under a specific owner +/// - Listing a user's followed crates +/// +/// Notes: +/// The different use cases this function covers is handled through passing +/// in parameters in the GET request. +/// +/// We would like to stop adding functionality in here. It was built like +/// this to keep the number of database queries low, though given Rust's +/// low performance overhead, this is a soft goal to have, and can afford +/// more database transactions if it aids understandability. +/// +/// All of the edge cases for this function are not currently covered +/// in testing, and if they fail, it is difficult to determine what +/// caused the break. In the future, we should look at splitting this +/// function out to cover the different use cases, and create unit tests +/// for them. pub fn index(req: &mut Request) -> CargoResult { use diesel::expression::{AsExpression, DayAndMonthIntervalDsl}; use diesel::types::{Bool, BigInt, Nullable}; @@ -913,6 +933,8 @@ pub fn index(req: &mut Request) -> CargoResult { )); } + // The database query returns a tuple within a tuple , with the root + // tuple containing 3 items. let data = query .paginate(limit, offset) .load::<((Crate, bool, Option), i64)>(&*conn)?; @@ -1100,6 +1122,8 @@ pub fn show(req: &mut Request) -> CargoResult { } /// Handles the `PUT /crates/new` route. +/// Used by `cargo publish` to publish a new crate or to publish a new version of an +/// existing crate. pub fn new(req: &mut Request) -> CargoResult { let app = req.app().clone(); let (new_crate, user) = parse_new_headers(req)?; @@ -1126,6 +1150,8 @@ pub fn new(req: &mut Request) -> CargoResult { let categories: Vec<_> = categories.iter().map(|k| &**k).collect(); let conn = req.db_conn()?; + // Create a transaction on the database, if there are no errors, + // commit the transactions to record a new or updated crate. conn.transaction(|| { // Persist the new crate, if it doesn't already exist let persist = NewCrate { @@ -1236,6 +1262,11 @@ pub fn new(req: &mut Request) -> CargoResult { }) } +/// Used by the `krate::new` function. +/// +/// This function parses the JSON headers to interpret the data and validates +/// the data during and after the parsing. Returns crate metadata and user +/// information. fn parse_new_headers(req: &mut Request) -> CargoResult<(upload::NewCrate, User)> { // Read the json upload request let amt = read_le_u32(req.body())? as u64; diff --git a/src/lib.rs b/src/lib.rs index ff6ef9b471b..0bd2e77ea7a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -148,11 +148,14 @@ pub fn middleware(app: Arc) -> MiddlewareBuilder { C(version::downloads), ); api_router.get("/crates/:crate_id/:version/authors", C(version::authors)); + // Used to generate download graphs api_router.get("/crates/:crate_id/downloads", C(krate::downloads)); api_router.get("/crates/:crate_id/versions", C(krate::versions)); api_router.put("/crates/:crate_id/follow", C(krate::follow)); api_router.delete("/crates/:crate_id/follow", C(krate::unfollow)); api_router.get("/crates/:crate_id/following", C(krate::following)); + // This endpoint may now be redundant, check frontend to see if it is + // being used api_router.get("/crates/:crate_id/owners", C(krate::owners)); api_router.get("/crates/:crate_id/owner_team", C(krate::owner_team)); api_router.get("/crates/:crate_id/owner_user", C(krate::owner_user)); diff --git a/src/upload.rs b/src/upload.rs index b12ebb3bb68..068b4b479f0 100644 --- a/src/upload.rs +++ b/src/upload.rs @@ -1,3 +1,7 @@ +//! This module handles the expected information a crate should have +//! and manages the serialising and deserialising of this information +//! to and from structs. The serlializing is only utilised in +//! integration tests. use std::collections::HashMap; use std::ops::Deref; diff --git a/src/version.rs b/src/version.rs index a2cc6268a84..bb3567294d2 100644 --- a/src/version.rs +++ b/src/version.rs @@ -461,6 +461,14 @@ pub fn authors(req: &mut Request) -> CargoResult { } /// Handles the `DELETE /crates/:crate_id/:version/yank` route. +/// This does not delete a crate version, it makes the crate +/// version accessible only to crates that already have a +/// `Cargo.lock` containing this version. +/// +/// Notes: +/// Crate deletion is not implemented to avoid breaking builds, +/// and the goal of yanking a crate is to prevent crates +/// beginning to depend on the yanked crate version. pub fn yank(req: &mut Request) -> CargoResult { modify_yank(req, true) } @@ -470,6 +478,7 @@ pub fn unyank(req: &mut Request) -> CargoResult { modify_yank(req, false) } +/// Changes `yanked` flag on a crate version record fn modify_yank(req: &mut Request, yanked: bool) -> CargoResult { let (version, krate) = version_and_crate(req)?; let user = req.user()?; From 25fe12ec1562ea12389ee20b5ce45ce23e2dfe11 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 12 Aug 2017 12:32:16 +1000 Subject: [PATCH 2/5] Add documentation comments for commit_and_push() function --- src/git.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/git.rs b/src/git.rs index a7489aacb90..d46ba885dcc 100644 --- a/src/git.rs +++ b/src/git.rs @@ -77,8 +77,6 @@ pub fn add_crate(app: &App, krate: &Crate) -> CargoResult<()> { /// file, deserlialise the crate from JSON, change the yank boolean to /// `true` or `false`, write all the lines back out, and commit and /// push the changes. -// TODO: factor out the tasks this function does into smaller, separate -// functions. pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> CargoResult<()> { let repo = app.git_repo.lock().unwrap(); let repo_path = repo.workdir().unwrap(); @@ -118,6 +116,21 @@ pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> }) } +/// Commits and pushes to the crates.io index. +/// +/// There are currently 2 instances of the crates.io backend running +/// on Heroku, and they race against each other e.g. if 2 pushes occur, +/// then one will succeed while the other will need to be rebased before +/// being pushed. +/// +/// A maximum of 20 attempts to commit and push to the index currently +/// accounts for the amount of traffic publishing crates, though this will +/// eventually need to be changed past a certain point. +/// +/// Notes: +/// Currently, this function is called from the HTTP thread and is blocking. +/// This could be changed to run from a different thread and use a callback +/// upon completion to the HTTP thread. fn commit_and_push(repo: &git2::Repository, mut f: F) -> CargoResult<()> where F: FnMut() -> CargoResult<(String, PathBuf)>, From ee4165118b13f50774cb7c6df56724c22eb45436 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 12 Aug 2017 17:15:54 +1000 Subject: [PATCH 3/5] cargo fmt --- src/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git.rs b/src/git.rs index d46ba885dcc..bd4b8767e6d 100644 --- a/src/git.rs +++ b/src/git.rs @@ -122,7 +122,7 @@ pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> /// on Heroku, and they race against each other e.g. if 2 pushes occur, /// then one will succeed while the other will need to be rebased before /// being pushed. -/// +/// /// A maximum of 20 attempts to commit and push to the index currently /// accounts for the amount of traffic publishing crates, though this will /// eventually need to be changed past a certain point. From 686a1aecdc249bf115006c500189ebbb90c27456 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Mon, 14 Aug 2017 11:22:56 +1000 Subject: [PATCH 4/5] Add notes about thread blocking --- src/git.rs | 11 ++++++----- src/krate.rs | 4 ++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/git.rs b/src/git.rs index bd4b8767e6d..3ddb164e4b4 100644 --- a/src/git.rs +++ b/src/git.rs @@ -124,13 +124,14 @@ pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> /// being pushed. /// /// A maximum of 20 attempts to commit and push to the index currently -/// accounts for the amount of traffic publishing crates, though this will -/// eventually need to be changed past a certain point. +/// accounts for the amount of traffic publishing crates, though this may +/// have to be changed in the future. /// /// Notes: -/// Currently, this function is called from the HTTP thread and is blocking. -/// This could be changed to run from a different thread and use a callback -/// upon completion to the HTTP thread. +/// Currently, this function is called on the HTTP thread and is blocking. +/// Spawning a separate thread for this function means that the request +/// can return without waiting for completion, and other methods of +/// notifying upon completion or error can be used. fn commit_and_push(repo: &git2::Repository, mut f: F) -> CargoResult<()> where F: FnMut() -> CargoResult<(String, PathBuf)>, diff --git a/src/krate.rs b/src/krate.rs index 9dbd880b676..b9445b8d72e 100644 --- a/src/krate.rs +++ b/src/krate.rs @@ -1124,6 +1124,10 @@ pub fn show(req: &mut Request) -> CargoResult { /// Handles the `PUT /crates/new` route. /// Used by `cargo publish` to publish a new crate or to publish a new version of an /// existing crate. +/// +/// Currently blocks the HTTP thread, perhaps some function calls can spawn new +/// threads and return completion or error through other methods a `cargo publish +/// --status` command, via crates.io's front end, or email. pub fn new(req: &mut Request) -> CargoResult { let app = req.app().clone(); let (new_crate, user) = parse_new_headers(req)?; From 838d957fc3d0b33d935a035a0f55f960e86de1f3 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Tue, 15 Aug 2017 14:20:34 +1000 Subject: [PATCH 5/5] cargo fmt --- src/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git.rs b/src/git.rs index 3ddb164e4b4..45e828ba206 100644 --- a/src/git.rs +++ b/src/git.rs @@ -124,7 +124,7 @@ pub fn yank(app: &App, krate: &str, version: &semver::Version, yanked: bool) -> /// being pushed. /// /// A maximum of 20 attempts to commit and push to the index currently -/// accounts for the amount of traffic publishing crates, though this may +/// accounts for the amount of traffic publishing crates, though this may /// have to be changed in the future. /// /// Notes: