models/version: Move bad_request() error transformation into controller code

The model layer is not supposed to know anything about HTTP API errors...

Author: Turbo87

src/controllers/krate/publish.rs

@@ -359,7 +359,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
             let hex_cksum: String = Sha256::digest(&tarball_bytes).encode_hex();
 
             // Persist the new version of this crate
-            let version = NewVersion::builder(krate.id, &version_string)
+            let new_version = NewVersion::builder(krate.id, &version_string)
                 .features(serde_json::to_value(&features)?)
                 .maybe_license(license.as_deref())
                 // Downcast is okay because the file length must be less than the max upload size
@@ -371,8 +371,18 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
                 .maybe_rust_version(rust_version.as_deref())
                 .has_lib(tarball_info.manifest.lib.is_some())
                 .bin_names(bin_names.as_slice())
-                .build()
-                .save(conn, &verified_email_address)?;
+                .build();
+
+            let version = match new_version.save(conn, &verified_email_address) {
+                Err(diesel::result::Error::DatabaseError(diesel::result::DatabaseErrorKind::UniqueViolation, _)) => {
+                    return Err(bad_request(format_args!(
+                        "crate version `{}` is already uploaded",
+                        new_version.num_no_build
+                    )));
+                },
+                Err(error) => return Err(error.into()),
+                Ok(version) => version,
+            };
 
             insert_version_owner_action(
                 conn,

src/models/version.rs

@@ -6,8 +6,6 @@ use crates_io_index::features::FeaturesMap;
 use diesel::prelude::*;
 use serde::Deserialize;
 
-use crate::util::errors::{bad_request, AppResult};
-
 use crate::models::{Crate, Dependency, User};
 use crate::schema::*;
 use crate::util::diesel::Conn;
@@ -85,7 +83,7 @@ pub struct NewVersion<'a> {
     #[builder(start_fn)]
     num: &'a str,
     #[builder(default = strip_build_metadata(num))]
-    num_no_build: &'a str,
+    pub num_no_build: &'a str,
     created_at: Option<&'a NaiveDateTime>,
     yanked: Option<bool>,
     #[builder(default = serde_json::Value::Object(Default::default()))]
@@ -102,21 +100,11 @@ pub struct NewVersion<'a> {
 }
 
 impl NewVersion<'_> {
-    pub fn save(&self, conn: &mut impl Conn, published_by_email: &str) -> AppResult<Version> {
+    pub fn save(&self, conn: &mut impl Conn, published_by_email: &str) -> QueryResult<Version> {
         use diesel::insert_into;
 
         conn.transaction(|conn| {
-            let version: Version = match insert_into(versions::table).values(self).get_result(conn)
-            {
-                Err(error) if is_unique_violation(&error) => {
-                    return Err(bad_request(format_args!(
-                        "crate version `{}` is already uploaded",
-                        self.num_no_build
-                    )));
-                }
-                Err(error) => return Err(error.into()),
-                Ok(version) => version,
-            };
+            let version: Version = insert_into(versions::table).values(self).get_result(conn)?;
 
             insert_into(versions_published_by::table)
                 .values((
@@ -136,11 +124,6 @@ fn strip_build_metadata(version: &str) -> &str {
         .unwrap_or(version)
 }
 
-fn is_unique_violation(error: &diesel::result::Error) -> bool {
-    use diesel::result::{DatabaseErrorKind::UniqueViolation, Error};
-    matches!(error, Error::DatabaseError(UniqueViolation, _))
-}
-
 /// The highest version (semver order) and the most recently updated version.
 /// Typically used for a single crate.
 #[derive(Debug, Clone, Eq, PartialEq)]