Merge #1436

1436: Add crate size on the crate detail page r=carols10cents a=PinkFloyded

From #1393

Currently it looks like this:

<img width="955" alt="crate_size_ui" src="https://user-images.githubusercontent.com/11929269/41512798-c7f2dede-72ad-11e8-98b4-aa4d29750678.png">

I'm new to Rust and Ember. So, let me know if something can be done better.

Co-authored-by: PinkFloyded <[email protected]>
Co-authored-by: Carol (Nichols || Goulding) <[email protected]>

Author: 3 people

app/helpers/format-crate-size.js

@@ -0,0 +1,11 @@
+import { helper } from '@ember/component/helper';
+
+export function formatCrateSize(sizeInBytes) {
+    if (sizeInBytes < 100000) {
+        return +(sizeInBytes / 1000).toFixed(2) + ' kB';
+    } else {
+        return +(sizeInBytes / 1000000).toFixed(2) + ' MB';
+    }
+}
+
+export default helper(formatCrateSize);

app/models/version.js

@@ -21,4 +21,5 @@ export default DS.Model.extend({
     crateName: computed('crate', function() {
         return this.belongsTo('crate').id();
     }),
+    crate_size: DS.attr('number'),
 });

app/styles/crate.scss

@@ -351,6 +351,25 @@
         font-weight: bold;
         margin-bottom: 40px;
     }
+
+    /*
+        Since crate_size is a new field, older crates won't have it.
+        Preserve behaviour for older crates. For newer ones, keep
+        `Crate Size` closer to last updated.
+    */
+    .date-with-small-margin-bot {
+        font-weight: bold;
+        margin-bottom: 20px;
+    }
+    .crate-size {
+        color: $main-color-light;
+        font-size: 90%;
+        line-height: 25px;
+    }
+    .size {
+        font-weight: bold;
+        margin-bottom: 40px;
+    }
 }
 
 #crate-downloads {

app/templates/crate/version.hbs

@@ -104,14 +104,21 @@
         <div class='top'>
             <div>
                 <div class='last-update'><span class='small'>Last Updated</span></div>
-                <div class='date'>{{moment-from-now crate.updated_at}}</div>
+                <div class='{{if currentVersion.crate_size 'date-with-small-margin-bot' 'date'}}'>{{moment-from-now crate.updated_at}}</div>
                 {{#each crate.annotated_badges as |badge|}}
                     <p>
                         {{component badge.component_name badge=badge}}
                     </p>
                 {{/each}}
             </div>
 
+            {{#if currentVersion.crate_size}}
+                <div>
+                    <div class='crate-size'><span class='small'>Crate Size</span></div>
+                    <div class='size'>{{format-crate-size currentVersion.crate_size}}</div>
+                </div>
+            {{/if}}
+
             <div class="authors">
                 <h3>Authors</h3>
                 <ul>

migrations/2018-06-03-195948_add_crate_size/down.sql

@@ -0,0 +1,2 @@
+ALTER TABLE versions
+DROP COLUMN crate_size;

migrations/2018-06-03-195948_add_crate_size/up.sql

@@ -0,0 +1,2 @@
+ALTER TABLE versions
+ADD COLUMN crate_size integer;

mirage/factories/version.js

@@ -27,4 +27,5 @@ export default Factory.extend({
         let crate = server.schema.crates.find(version.crate);
         crate.update({ versions: crate.versions.concat(parseInt(version.id, 10)) });
     },
+    crate_size: () => faker.random.number({ max: 10000000 }),
 });

mirage/fixtures/versions.js

@@ -13,6 +13,7 @@ export default [
         updated_at: '2016-12-27T08:40:00Z',
         yanked: false,
         license: 'MIT',
+        crate_size: 912355,
         _authors: [
             'Daniel Fagnan <[email protected]>',
             'Jason E. Aten',
@@ -34,6 +35,7 @@ export default [
         },
         id: 40905,
         num: '0.6.1',
+        crate_size: 8123545,
         updated_at: '2016-12-27T08:40:00Z',
         yanked: false,
         license: 'Apache-2.0',

mirage/serializers/version.js

@@ -13,6 +13,7 @@ export default BaseSerializer.extend({
         'updated_at',
         'yanked',
         'license',
+        'crate_size',
     ],
 
     links(version) {

src/bin/update-downloads.rs

@@ -145,6 +145,7 @@ mod test {
             &HashMap::new(),
             None,
             None,
+            None,
         ).unwrap();
         let version = version.save(&conn, &[]).unwrap();
         (krate, version)

src/controllers/krate/publish.rs

@@ -1,6 +1,5 @@
 //! Functionality related to publishing a new crate or version of a crate.
 
-use std::cmp;
 use std::collections::HashMap;
 use std::sync::Arc;
 
@@ -9,7 +8,7 @@ use serde_json;
 
 use git;
 use render;
-use util::{internal, ChainError};
+use util::{internal, ChainError, Maximums};
 use util::{read_fill, read_le_u32};
 
 use controllers::prelude::*;
@@ -26,6 +25,20 @@ use views::{EncodableCrate, EncodableCrateUpload};
 /// --status` command, via crates.io's front end, or email.
 pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
     let app = Arc::clone(req.app());
+
+    // The format of the req.body() of a publish request is as follows:
+    //
+    // metadata length
+    // metadata in JSON about the crate being published
+    // .crate tarball length
+    // .crate tarball file
+    //
+    // - The metadata is read and interpreted in the parse_new_headers function.
+    // - The .crate tarball length is read in this function in order to save the size of the file
+    //   in the version record in the database.
+    // - Then the .crate tarball length is passed to the upload_crate function where the actual
+    //   file is read and uploaded.
+
     let (new_crate, user) = parse_new_headers(req)?;
 
     let name = &*new_crate.name;
@@ -88,23 +101,43 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
             )));
         }
 
-        let length = req
+        // Length of the .crate tarball, which appears after the metadata in the request body.
+        // TODO: Not sure why we're using the total content length (metadata + .crate file length)
+        // to compare against the max upload size... investigate that and perhaps change to use
+        // this file length.
+        let file_length = read_le_u32(req.body())?;
+
+        let content_length = req
             .content_length()
             .chain_error(|| human("missing header: Content-Length"))?;
-        let max = krate
-            .max_upload_size
-            .map(|m| m as u64)
-            .unwrap_or(app.config.max_upload_size);
-        if length > max {
-            return Err(human(&format_args!("max upload size is: {}", max)));
+
+        let maximums = Maximums::new(
+            krate.max_upload_size,
+            app.config.max_upload_size,
+            app.config.max_unpack_size,
+        );
+
+        if content_length > maximums.max_upload_size {
+            return Err(human(&format_args!(
+                "max upload size is: {}",
+                maximums.max_upload_size
+            )));
         }
 
         // This is only redundant for now. Eventually the duplication will be removed.
         let license = new_crate.license.clone();
 
         // Persist the new version of this crate
-        let version = NewVersion::new(krate.id, vers, &features, license, license_file)?
-            .save(&conn, &new_crate.authors)?;
+        let version = NewVersion::new(
+            krate.id,
+            vers,
+            &features,
+            license,
+            license_file,
+            // Downcast is okay because the file length must be less than the max upload size
+            // to get here, and max upload sizes are way less than i32 max
+            Some(file_length as i32),
+        )?.save(&conn, &new_crate.authors)?;
 
         // Link this new version to all dependencies
         let git_deps = dependency::add_dependencies(&conn, &new_crate.deps, version.id)?;
@@ -134,11 +167,10 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
         // Upload the crate, return way to delete the crate from the server
         // If the git commands fail below, we shouldn't keep the crate on the
         // server.
-        let max_unpack = cmp::max(app.config.max_unpack_size, max);
-        let (cksum, mut crate_bomb, mut readme_bomb) = app
-            .config
-            .uploader
-            .upload_crate(req, &krate, readme, max, max_unpack, vers)?;
+        let (cksum, mut crate_bomb, mut readme_bomb) =
+            app.config
+                .uploader
+                .upload_crate(req, &krate, readme, file_length, maximums, vers)?;
         version.record_readme_rendering(&conn)?;
 
         let mut hex_cksum = String::new();
@@ -195,12 +227,12 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
 /// information.
 fn parse_new_headers(req: &mut dyn Request) -> CargoResult<(EncodableCrateUpload, User)> {
     // Read the json upload request
-    let amt = u64::from(read_le_u32(req.body())?);
+    let metadata_length = u64::from(read_le_u32(req.body())?);
     let max = req.app().config.max_upload_size;
-    if amt > max {
+    if metadata_length > max {
         return Err(human(&format_args!("max upload size is: {}", max)));
     }
-    let mut json = vec![0; amt as usize];
+    let mut json = vec![0; metadata_length as usize];
     read_fill(req.body(), &mut json)?;
     let json = String::from_utf8(json).map_err(|_| human("json body was not valid utf-8"))?;
     let new: EncodableCrateUpload = serde_json::from_str(&json)

src/lib.rs

@@ -6,7 +6,7 @@
 #![deny(warnings)]
 #![deny(missing_debug_implementations, missing_copy_implementations)]
 #![deny(bare_trait_objects)]
-#![recursion_limit = "128"]
+#![recursion_limit = "256"]
 #![allow(unknown_lints, proc_macro_derive_resolution_fallback)] // This can be removed after diesel-1.4
 
 extern crate ammonia;

src/models/version.rs

@@ -27,6 +27,7 @@ pub struct Version {
     pub features: serde_json::Value,
     pub yanked: bool,
     pub license: Option<String>,
+    pub crate_size: Option<i32>,
 }
 
 #[derive(Insertable, Debug)]
@@ -36,6 +37,7 @@ pub struct NewVersion {
     num: String,
     features: serde_json::Value,
     license: Option<String>,
+    crate_size: Option<i32>,
 }
 
 impl Version {
@@ -49,6 +51,7 @@ impl Version {
             features,
             yanked,
             license,
+            crate_size,
             ..
         } = self;
         let num = num.to_string();
@@ -69,6 +72,7 @@ impl Version {
                 version_downloads: format!("/api/v1/crates/{}/{}/downloads", crate_name, num),
                 authors: format!("/api/v1/crates/{}/{}/authors", crate_name, num),
             },
+            crate_size,
         }
     }
 
@@ -117,6 +121,7 @@ impl NewVersion {
         features: &HashMap<String, Vec<String>>,
         license: Option<String>,
         license_file: Option<&str>,
+        crate_size: Option<i32>,
     ) -> CargoResult<Self> {
         let features = serde_json::to_value(features)?;
 
@@ -125,6 +130,7 @@ impl NewVersion {
             num: num.to_string(),
             features,
             license,
+            crate_size,
         };
 
         new_version.validate_license(license_file)?;
@@ -200,6 +206,7 @@ impl Queryable<versions::SqlType, Pg> for Version {
         serde_json::Value,
         bool,
         Option<String>,
+        Option<i32>,
     );
 
     fn build(row: Self::Row) -> Self {
@@ -213,6 +220,7 @@ impl Queryable<versions::SqlType, Pg> for Version {
             features: row.6,
             yanked: row.7,
             license: row.8,
+            crate_size: row.9,
         }
     }
 }

src/schema.rs

@@ -864,6 +864,12 @@ table! {
         ///
         /// (Automatically generated by Diesel.)
         license -> Nullable<Varchar>,
+        /// The `crate_size` column of the `versions` table.
+        ///
+        /// Its SQL type is `Nullable<Int4>`.
+        ///
+        /// (Automatically generated by Diesel.)
+        crate_size -> Nullable<Int4>,
     }
 }
 

src/tests/all.rs

@@ -45,7 +45,7 @@ use models::{Crate, CrateDownload, CrateOwner, Dependency, Keyword, Team, User,
 use models::{NewCategory, NewCrate, NewTeam, NewUser, NewVersion};
 use schema::*;
 use views::krate_publish as u;
-use views::EncodableCrate;
+use views::{EncodableCrate, EncodableKeyword, EncodableVersion};
 
 macro_rules! t {
     ($e:expr) => {
@@ -125,6 +125,13 @@ struct Warnings {
 struct CrateMeta {
     total: i32,
 }
+#[derive(Deserialize)]
+struct CrateResponse {
+    #[serde(rename = "crate")]
+    krate: EncodableCrate,
+    versions: Vec<EncodableVersion>,
+    keywords: Vec<EncodableKeyword>,
+}
 
 fn app() -> (
     record::Bomb,
@@ -160,7 +167,7 @@ fn app() -> (
         gh_client_secret: env::var("GH_CLIENT_SECRET").unwrap_or_default(),
         db_url: env("TEST_DATABASE_URL"),
         env: cargo_registry::Env::Test,
-        max_upload_size: 1000,
+        max_upload_size: 3000,
         max_unpack_size: 2000,
         mirror: Replica::Primary,
         api_protocol: api_protocol,
@@ -321,6 +328,7 @@ impl<'a> VersionBuilder<'a> {
             &self.features,
             license,
             self.license_file,
+            None,
         )?.save(connection, &[])?;
 
         if self.yanked {
@@ -493,9 +501,9 @@ impl<'a> CrateBuilder<'a> {
     }
 }
 
-fn new_version(crate_id: i32, num: &str) -> NewVersion {
+fn new_version(crate_id: i32, num: &str, crate_size: Option<i32>) -> NewVersion {
     let num = semver::Version::parse(num).unwrap();
-    NewVersion::new(crate_id, &num, &HashMap::new(), None, None).unwrap()
+    NewVersion::new(crate_id, &num, &HashMap::new(), None, None, crate_size).unwrap()
 }
 
 fn krate(name: &str) -> Crate {
@@ -516,6 +524,28 @@ fn krate(name: &str) -> Crate {
     }
 }
 
+fn new_crate(name: &str, version: &str) -> u::NewCrate {
+    u::NewCrate {
+        name: u::CrateName(name.to_string()),
+        vers: u::CrateVersion(semver::Version::parse(version).unwrap()),
+        features: HashMap::new(),
+        deps: Vec::new(),
+        authors: vec!["foo".to_string()],
+        description: Some("desc".to_string()),
+        homepage: None,
+        documentation: None,
+        readme: None,
+        readme_file: None,
+        keywords: None,
+        categories: None,
+        license: Some("MIT".to_string()),
+        license_file: None,
+        repository: None,
+        badges: None,
+        links: None,
+    }
+}
+
 fn sign_in_as(req: &mut Request, user: &User) {
     req.mut_extensions().insert(user.clone());
     req.mut_extensions()