Merge #931 #936 #969 #979

931: Touch a crate whenever its versions are changed r=carols10cents

Prior to this commit, if uploading a new version of a crate didn't
otherwise change the metadata (description, etc), the crate's updated_at
record wouldn't be changed.

The migration specifically checks if `updated_at` changed on the
versions to make sure the touch doesn't happen if `downloads` was
bumped. I wanted to avoid duplicating that here, so instead this will
only happen if the row is new, or as a result of the trigger bumping
updated_at on the version.

I have specifically not included a query to correct existing data, as
it's an irreversable operation. We should manually run a query like
`UPDATE crates SET updated_at = MAX(crates.updated_at,
SELECT(MAX(versions.updated_at) FROM versions WHERE versions.crate_id =
crates.id))` if we want to correct existing data.

Fixes #908.
Close #925.

936: Add more documentation r=carols10cents

As part of the effort to better document crates.io, I added more documentation comments to the `krate::index` and `krate::new` functions as well as to some associated functions. Also added some comments in some functions and to document. Thanks for providing the background on these functions @carols10cents :).

I'd like to doc comment the git::yank function as well before this gets merged.

969: crate.scss: When rendering code in README, don't show scrollbar unless necessary r=carols10cents

The current `overflow-x: scroll` shows a scrollbar all the time, even
for code blocks that don't need them. Switch to `overflow-x: auto`, to
only show the scrollbar when needed.

979: CSS: Restrict readme image width r=carols10cents

Fixes #971

Author: bors-voyager[bot]

app/styles/crate.scss

@@ -306,8 +306,12 @@
     .crate-readme {
         line-height: 1.5;
 
+        img {
+            max-width: 100%;
+        }
+
         pre {
-            overflow-x: scroll;
+            overflow-x: auto;
         }
     }
     .last-update {

migrations/20170801133919_touch_crates_when_versions_modified/down.sql

@@ -0,0 +1,2 @@
+DROP TRIGGER touch_crate ON versions;
+DROP FUNCTION touch_crate_on_version_modified();

migrations/20170801133919_touch_crates_when_versions_modified/up.sql

@@ -0,0 +1,15 @@
+CREATE OR REPLACE FUNCTION touch_crate_on_version_modified() RETURNS trigger AS $$
+BEGIN
+  IF (
+    TG_OP = 'INSERT' OR
+    NEW.updated_at IS DISTINCT FROM OLD.updated_at
+  ) THEN
+    UPDATE crates SET updated_at = CURRENT_TIMESTAMP WHERE
+      crates.id = NEW.crate_id;
+  END IF;
+  RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE TRIGGER touch_crate BEFORE INSERT OR UPDATE ON versions
+  FOR EACH ROW EXECUTE PROCEDURE touch_crate_on_version_modified();

src/git.rs

@@ -73,6 +73,10 @@ 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.
 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();
@@ -112,6 +116,22 @@ 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 may
+/// have to be changed in the future.
+///
+/// Notes:
+/// 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<F>(repo: &git2::Repository, mut f: F) -> CargoResult<()>
 where
     F: FnMut() -> CargoResult<(String, PathBuf)>,

src/krate.rs

@@ -548,6 +548,26 @@ impl 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<Response> {
     use diesel::expression::{AsExpression, DayAndMonthIntervalDsl};
     use diesel::types::{Bool, BigInt, Nullable};
@@ -676,6 +696,8 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
         ));
     }
 
+    // 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>), i64)>(&*conn)?;
@@ -863,6 +885,12 @@ pub fn show(req: &mut Request) -> CargoResult<Response> {
 }
 
 /// 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<Response> {
     let app = req.app().clone();
     let (new_crate, user) = parse_new_headers(req)?;
@@ -889,6 +917,8 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
     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 {
@@ -1012,6 +1042,11 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
     })
 }
 
+/// 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;

src/lib.rs

@@ -148,11 +148,14 @@ pub fn middleware(app: Arc<App>) -> 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));

src/schema.rs

@@ -183,3 +183,5 @@ table! {
         license -> Nullable<Varchar>,
     }
 }
+
+operator_allowed!(crates::updated_at, Sub, sub);

src/tests/http-data/krate_uploading_new_version_touches_crate

@@ -0,0 +1,42 @@
+===REQUEST 371
+PUT http://alexcrichton-test.s3.amazonaws.com/crates/foo_versions_updated_at/foo_versions_updated_at-1.0.0.crate HTTP/1.1
+Accept: */*
+Proxy-Connection: Keep-Alive
+Date: Sun, 28 Jun 2015 14:07:18 -0700
+Content-Type: application/x-tar
+Content-Length: 0
+Host: alexcrichton-test.s3.amazonaws.com
+Authorization: AWS AKIAJF3GEK7N44BACDZA:B6cKWtg9t24ej4DljrlsMnkgtdg=
+
+
+===RESPONSE 258
+HTTP/1.1 200
+x-amz-request-id: D72B3AB3755D34E3
+etag: "d41d8cd98f00b204e9800998ecf8427e"
+date: Sun, 28 Jun 2015 21:07:51 GMT
+x-amz-id-2: 4MEDTSaUXJ7J70JfbXTcfI2m1qoriY+OEOxVAmj2oARpllZTI+vCT52o9FZnVUzJzkchRL407nI=
+content-length: 0
+server: AmazonS3
+
+
+===REQUEST 371
+PUT http://alexcrichton-test.s3.amazonaws.com/crates/foo_versions_updated_at/foo_versions_updated_at-2.0.0.crate HTTP/1.1
+Accept: */*
+Proxy-Connection: Keep-Alive
+Date: Sun, 28 Jun 2015 14:07:18 -0700
+Content-Type: application/x-tar
+Content-Length: 0
+Host: alexcrichton-test.s3.amazonaws.com
+Authorization: AWS AKIAJF3GEK7N44BACDZA:B6cKWtg9t24ej4DljrlsMnkgtdg=
+
+
+===RESPONSE 258
+HTTP/1.1 200
+x-amz-request-id: D72B3AB3755D34E3
+etag: "d41d8cd98f00b204e9800998ecf8427e"
+date: Sun, 28 Jun 2015 21:07:51 GMT
+x-amz-id-2: 4MEDTSaUXJ7J70JfbXTcfI2m1qoriY+OEOxVAmj2oARpllZTI+vCT52o9FZnVUzJzkchRL407nI=
+content-length: 0
+server: AmazonS3
+
+

src/tests/krate.rs

@@ -19,7 +19,7 @@ use cargo_registry::krate::{Crate, EncodableCrate, MAX_NAME_LENGTH};
 
 use cargo_registry::token::ApiToken;
 use cargo_registry::owner::EncodableOwner;
-use cargo_registry::schema::versions;
+use cargo_registry::schema::{versions, crates};
 
 use cargo_registry::upload as u;
 use cargo_registry::user::EncodablePublicUser;
@@ -463,6 +463,44 @@ fn versions() {
     assert_eq!(json.versions[2].num, "0.5.0");
 }
 
+#[test]
+fn uploading_new_version_touches_crate() {
+    use diesel::expression::dsl::*;
+
+    let (_b, app, middle) = ::app();
+
+    let mut upload_req = ::new_req(app.clone(), "foo_versions_updated_at", "1.0.0");
+    let u = ::sign_in(&mut upload_req, &app);
+    ok_resp!(middle.call(&mut upload_req));
+
+    {
+        let conn = app.diesel_database.get().unwrap();
+        diesel::update(crates::table)
+            .set(crates::updated_at.eq(crates::updated_at - 1.hour()))
+            .execute(&*conn)
+            .unwrap();
+    }
+
+    let mut show_req = ::req(
+        app.clone(),
+        Method::Get,
+        "/api/v1/crates/foo_versions_updated_at",
+    );
+    let mut response = ok_resp!(middle.call(&mut show_req));
+    let json: CrateResponse = ::json(&mut response);
+    let updated_at_before = json.krate.updated_at;
+
+    let mut upload_req = ::new_req(app.clone(), "foo_versions_updated_at", "2.0.0");
+    ::sign_in_as(&mut upload_req, &u);
+    ok_resp!(middle.call(&mut upload_req));
+
+    let mut response = ok_resp!(middle.call(&mut show_req));
+    let json: CrateResponse = ::json(&mut response);
+    let updated_at_after = json.krate.updated_at;
+
+    assert_ne!(updated_at_before, updated_at_after);
+}
+
 #[test]
 fn new_wrong_token() {
     let (_b, app, middle) = ::app();

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;
 

src/version.rs

@@ -388,6 +388,14 @@ pub fn authors(req: &mut Request) -> CargoResult<Response> {
 }
 
 /// 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<Response> {
     modify_yank(req, true)
 }
@@ -397,6 +405,7 @@ pub fn unyank(req: &mut Request) -> CargoResult<Response> {
     modify_yank(req, false)
 }
 
+/// Changes `yanked` flag on a crate version record
 fn modify_yank(req: &mut Request, yanked: bool) -> CargoResult<Response> {
     let (version, krate) = version_and_crate(req)?;
     let user = req.user()?;