Auto merge of #3113 - Turbo87:top-version-cleanup, r=jtgeibel

Clean up `TopVersions` code

This PR cleans up the code that figures out the `highest` and `newest` versions of a crate in several ways. I hope that the commit list gives enough context on the individual changes.

Primarily, this decouples the code from the `Version` struct, and it moves the `0.0.0` defaulting to the `encodable()` method of the `Krate` model, instead of performing that in the low-level code directly.

Like #3067, this PR is also in preparation for resolving #2710.

r? `@jtgeibel`

Author: bors

src/controllers/krate/metadata.rs

@@ -9,7 +9,7 @@ use crate::controllers::helpers::pagination::PaginationOptions;
 
 use crate::models::{
     Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads,
-    User, Version, VersionOwnerAction,
+    TopVersions, User, Version, VersionOwnerAction,
 };
 use crate::schema::*;
 use crate::views::{
@@ -37,7 +37,7 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult {
         versions
             .grouped_by(&krates)
             .into_iter()
-            .map(|versions| Version::top(versions.into_iter().map(|v| (v.created_at, v.num))))
+            .map(TopVersions::from_versions)
             .zip(krates)
             .zip(recent_downloads)
             .map(|((top_versions, krate), recent_downloads)| {

src/controllers/krate/search.rs

@@ -6,7 +6,9 @@ use diesel_full_text_search::*;
 use crate::controllers::cargo_prelude::*;
 use crate::controllers::helpers::Paginate;
 use crate::controllers::util::AuthenticatedUser;
-use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version};
+use crate::models::{
+    Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, TopVersions, Version,
+};
 use crate::schema::*;
 use crate::util::errors::{bad_request, ChainError};
 use crate::views::EncodableCrate;
@@ -207,7 +209,7 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
     let versions = versions
         .grouped_by(&crates)
         .into_iter()
-        .map(|versions| Version::top(versions.into_iter().map(|v| (v.created_at, v.num))));
+        .map(TopVersions::from_versions);
 
     let badges: Vec<CrateBadge> = CrateBadge::belonging_to(&crates)
         .select((badges::crate_id, badges::all_columns))

src/models.rs

@@ -13,7 +13,7 @@ pub use self::rights::Rights;
 pub use self::team::{NewTeam, Team};
 pub use self::token::{ApiToken, CreatedApiToken};
 pub use self::user::{NewUser, User};
-pub use self::version::{NewVersion, Version};
+pub use self::version::{NewVersion, TopVersions, Version};
 
 pub mod helpers;
 

src/models/krate.rs

@@ -345,6 +345,18 @@ impl Crate {
         let badges = badges.map(|bs| bs.into_iter().map(Badge::into).collect());
         let documentation = Crate::remove_blocked_documentation_urls(documentation);
 
+        let max_version = top_versions
+            .highest
+            .as_ref()
+            .map(|v| v.to_string())
+            .unwrap_or_else(|| "0.0.0".to_string());
+
+        let newest_version = top_versions
+            .newest
+            .as_ref()
+            .map(|v| v.to_string())
+            .unwrap_or_else(|| "0.0.0".to_string());
+
         EncodableCrate {
             id: name.clone(),
             name: name.clone(),
@@ -356,8 +368,8 @@ impl Crate {
             keywords: keyword_ids,
             categories: category_ids,
             badges,
-            max_version: top_versions.highest.to_string(),
-            newest_version: top_versions.newest.to_string(),
+            max_version,
+            newest_version,
             documentation,
             homepage,
             exact_match,
@@ -407,7 +419,7 @@ impl Crate {
     pub fn top_versions(&self, conn: &PgConnection) -> QueryResult<TopVersions> {
         use crate::schema::versions::dsl::*;
 
-        Ok(Version::top(
+        Ok(TopVersions::from_date_version_pairs(
             self.versions().select((updated_at, num)).load(conn)?,
         ))
     }

src/models/version.rs

@@ -41,18 +41,29 @@ pub struct NewVersion {
 /// Typically used for a single crate.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct TopVersions {
-    pub highest: semver::Version,
-    pub newest: semver::Version,
+    /// The "highest" version in terms of semver
+    pub highest: Option<semver::Version>,
+    /// The "newest" version in terms of publishing date
+    pub newest: Option<semver::Version>,
 }
 
-/// A default semver value, "0.0.0", for use in TopVersions
-fn default_semver_version() -> semver::Version {
-    semver::Version {
-        major: 0,
-        minor: 0,
-        patch: 0,
-        pre: vec![],
-        build: vec![],
+impl TopVersions {
+    /// Return both the newest (most recently updated) and the
+    /// highest version (in semver order) for a list of `Version` instances.
+    pub fn from_versions(versions: Vec<Version>) -> Self {
+        Self::from_date_version_pairs(versions.into_iter().map(|v| (v.created_at, v.num)))
+    }
+
+    /// Return both the newest (most recently updated) and the
+    /// highest version (in semver order) for a collection of date/version pairs.
+    pub fn from_date_version_pairs<T>(pairs: T) -> Self
+    where
+        T: Clone + IntoIterator<Item = (NaiveDateTime, semver::Version)>,
+    {
+        let newest = pairs.clone().into_iter().max().map(|(_, v)| v);
+        let highest = pairs.into_iter().map(|(_, v)| v).max();
+
+        Self { newest, highest }
     }
 }
 
@@ -115,30 +126,6 @@ impl Version {
             .load(conn)
     }
 
-    /// Return both the newest (most recently updated) and the
-    /// highest version (in semver order) for a collection of date/version pairs.
-    pub fn top<T>(pairs: T) -> TopVersions
-    where
-        T: Clone + IntoIterator<Item = (NaiveDateTime, semver::Version)>,
-    {
-        TopVersions {
-            newest: pairs
-                .clone()
-                .into_iter()
-                .max()
-                .unwrap_or((
-                    NaiveDateTime::from_timestamp(0, 0),
-                    default_semver_version(),
-                ))
-                .1,
-            highest: pairs
-                .into_iter()
-                .map(|(_, v)| v)
-                .max()
-                .unwrap_or_else(default_semver_version),
-        }
-    }
-
     pub fn record_readme_rendering(version_id_: i32, conn: &PgConnection) -> QueryResult<usize> {
         use crate::schema::readme_renderings::dsl::*;
         use diesel::dsl::now;
@@ -255,7 +242,7 @@ impl NewVersion {
 
 #[cfg(test)]
 mod tests {
-    use super::{TopVersions, Version};
+    use super::TopVersions;
     use chrono::NaiveDateTime;
 
     #[track_caller]
@@ -272,10 +259,10 @@ mod tests {
     fn top_versions_empty() {
         let versions = vec![];
         assert_eq!(
-            Version::top(versions),
+            TopVersions::from_date_version_pairs(versions),
             TopVersions {
-                highest: version("0.0.0"),
-                newest: version("0.0.0"),
+                highest: None,
+                newest: None,
             }
         );
     }
@@ -284,10 +271,10 @@ mod tests {
     fn top_versions_single() {
         let versions = vec![(date("2020-12-03T12:34:56"), version("1.0.0"))];
         assert_eq!(
-            Version::top(versions),
+            TopVersions::from_date_version_pairs(versions),
             TopVersions {
-                highest: version("1.0.0"),
-                newest: version("1.0.0"),
+                highest: Some(version("1.0.0")),
+                newest: Some(version("1.0.0")),
             }
         );
     }
@@ -300,10 +287,10 @@ mod tests {
             (date("2020-12-03T12:34:56"), version("1.1.0")),
         ];
         assert_eq!(
-            Version::top(versions),
+            TopVersions::from_date_version_pairs(versions),
             TopVersions {
-                highest: version("2.0.0-alpha.1"),
-                newest: version("1.1.0"),
+                highest: Some(version("2.0.0-alpha.1")),
+                newest: Some(version("1.1.0")),
             }
         );
     }