Auto merge of #1815 - rye:remove-crates-license-column, r=sgrif

Remove `license` column on `crates` table

Seemingly the last step in the checklist on #736, this PR removes the `license` column on the `crates` table, and all references thereto. (In fact, I think #736 can be closed once this PR is merged.)

As of #803, the front-end no longer really used this field, and the front-end was also changed to render the license of the _current_ version, instead of the value coming from this now-obsolete `license` column on the crate. Thus, this column likely contains stale data and, to my knowledge, isn't changed at all when a new version is uploaded.

I wasn't sure what requirements are enforced for backwards-compatibility, or whether my changes here would actually affect behavior of e.g. if there are old versions of `cargo` that will upload with these values. I am happy for these changes to be scrutinized until this is verified to be safe.

Author: bors

migrations/2019-08-28-224353_remove_licenses_column_from_crates/down.sql

@@ -0,0 +1,2 @@
+-- This file should undo anything in `up.sql`
+ALTER TABLE crates ADD COLUMN license VARCHAR;

migrations/2019-08-28-224353_remove_licenses_column_from_crates/up.sql

@@ -0,0 +1,3 @@
+-- ALERT! DESTRUCTIVE MIGRATION
+-- DO NOT DEPLOY UNTIL PREVIOUS COMMIT IS DEPLOYED
+ALTER TABLE crates DROP COLUMN license;

src/controllers/krate/publish.rs

@@ -79,17 +79,12 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
             documentation: new_crate.documentation.as_ref().map(|s| &**s),
             readme: new_crate.readme.as_ref().map(|s| &**s),
             repository: repo.as_ref().map(String::as_str),
-            license: new_crate.license.as_ref().map(|s| &**s),
             max_upload_size: None,
         };
 
         let license_file = new_crate.license_file.as_ref().map(|s| &**s);
-        let krate = persist.create_or_update(
-            &conn,
-            license_file,
-            user.id,
-            Some(&app.config.publish_rate_limit),
-        )?;
+        let krate =
+            persist.create_or_update(&conn, user.id, Some(&app.config.publish_rate_limit))?;
 
         let owners = krate.owners(&conn)?;
         if user.rights(req.app(), &owners)? < Rights::Publish {

src/models/krate.rs

@@ -43,7 +43,6 @@ pub struct Crate {
     pub description: Option<String>,
     pub homepage: Option<String>,
     pub documentation: Option<String>,
-    pub license: Option<String>,
     pub repository: Option<String>,
     pub max_upload_size: Option<i32>,
 }
@@ -59,7 +58,6 @@ type AllColumns = (
     crates::description,
     crates::homepage,
     crates::documentation,
-    crates::license,
     crates::repository,
     crates::max_upload_size,
 );
@@ -73,7 +71,6 @@ pub const ALL_COLUMNS: AllColumns = (
     crates::description,
     crates::homepage,
     crates::documentation,
-    crates::license,
     crates::repository,
     crates::max_upload_size,
 );
@@ -98,20 +95,18 @@ pub struct NewCrate<'a> {
     pub readme: Option<&'a str>,
     pub repository: Option<&'a str>,
     pub max_upload_size: Option<i32>,
-    pub license: Option<&'a str>,
 }
 
 impl<'a> NewCrate<'a> {
     pub fn create_or_update(
         mut self,
         conn: &PgConnection,
-        license_file: Option<&'a str>,
         uploader: i32,
         rate_limit: Option<&PublishRateLimit>,
     ) -> CargoResult<Crate> {
         use diesel::update;
 
-        self.validate(license_file)?;
+        self.validate()?;
         self.ensure_name_not_reserved(conn)?;
 
         conn.transaction(|| {
@@ -133,7 +128,7 @@ impl<'a> NewCrate<'a> {
         })
     }
 
-    fn validate(&mut self, license_file: Option<&'a str>) -> CargoResult<()> {
+    fn validate(&mut self) -> CargoResult<()> {
         fn validate_url(url: Option<&str>, field: &str) -> CargoResult<()> {
             let url = match url {
                 Some(s) => s,
@@ -164,28 +159,6 @@ impl<'a> NewCrate<'a> {
         validate_url(self.homepage, "homepage")?;
         validate_url(self.documentation, "documentation")?;
         validate_url(self.repository, "repository")?;
-        self.validate_license(license_file)?;
-        Ok(())
-    }
-
-    fn validate_license(&mut self, license_file: Option<&str>) -> CargoResult<()> {
-        if let Some(license) = self.license {
-            for part in license.split('/') {
-                license_exprs::validate_license_expr(part).map_err(|e| {
-                    human(&format_args!(
-                        "{}; see http://opensource.org/licenses \
-                         for options, and http://spdx.org/licenses/ \
-                         for their identifiers",
-                        e
-                    ))
-                })?;
-            }
-        } else if license_file.is_some() {
-            // If no license is given, but a license file is given, flag this
-            // crate as having a nonstandard license. Note that we don't
-            // actually do anything else with license_file currently.
-            self.license = Some("non-standard");
-        }
         Ok(())
     }
 

src/models/token.rs

@@ -96,5 +96,4 @@ mod tests {
             .find(r#""last_used_at":"2017-01-06T14:23:12+00:00""#)
             .is_some());
     }
-
 }

src/schema.rs

@@ -332,12 +332,6 @@ table! {
         ///
         /// (Automatically generated by Diesel.)
         textsearchable_index_col -> Tsvector,
-        /// The `license` column of the `crates` table.
-        ///
-        /// Its SQL type is `Nullable<Varchar>`.
-        ///
-        /// (Automatically generated by Diesel.)
-        license -> Nullable<Varchar>,
         /// The `repository` column of the `crates` table.
         ///
         /// Its SQL type is `Nullable<Varchar>`.

src/tasks/update_downloads.rs

@@ -103,7 +103,7 @@ mod test {
             name: "foo",
             ..Default::default()
         }
-        .create_or_update(conn, None, user_id, None)
+        .create_or_update(conn, user_id, None)
         .unwrap();
         let version = NewVersion::new(
             krate.id,

src/tests/builders.rs

@@ -234,7 +234,7 @@ impl<'a> CrateBuilder<'a> {
 
         let mut krate = self
             .krate
-            .create_or_update(connection, None, self.owner_id, None)?;
+            .create_or_update(connection, self.owner_id, None)?;
 
         // Since we are using `NewCrate`, we can't set all the
         // crate properties in a single DB call.