Merge pull request #9756 from Turbo87/num_no_build

Fix build metadata race condition

Author: Turbo87

migrations/2024-10-25-112826_make-unique-version-unique/down.sql

@@ -0,0 +1,2 @@
+alter table versions
+    drop constraint versions_crate_id_num_no_build_uindex;

migrations/2024-10-25-112826_make-unique-version-unique/metadata.toml

@@ -0,0 +1 @@
+run_in_transaction = false

migrations/2024-10-25-112826_make-unique-version-unique/up.sql

@@ -0,0 +1,2 @@
+create unique index concurrently if not exists versions_crate_id_num_no_build_uindex
+    on versions (crate_id, num_no_build);

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,11 +6,8 @@ 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::sql::split_part;
 use crate::util::diesel::Conn;
 
 // Queryable has a custom implementation below
@@ -86,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()))]
@@ -103,24 +100,10 @@ pub struct NewVersion<'a> {
 }
 
 impl NewVersion<'_> {
-    pub fn save(&self, conn: &mut impl Conn, published_by_email: &str) -> AppResult<Version> {
-        use diesel::dsl::exists;
-        use diesel::{insert_into, select};
+    pub fn save(&self, conn: &mut impl Conn, published_by_email: &str) -> QueryResult<Version> {
+        use diesel::insert_into;
 
         conn.transaction(|conn| {
-            let num_no_build = strip_build_metadata(self.num);
-
-            let already_uploaded = versions::table
-                .filter(versions::crate_id.eq(self.crate_id))
-                .filter(split_part(versions::num, "+", 1).eq(num_no_build));
-
-            if select(exists(already_uploaded)).get_result(conn)? {
-                return Err(bad_request(format_args!(
-                    "crate version `{}` is already uploaded",
-                    num_no_build
-                )));
-            }
-
             let version: Version = insert_into(versions::table).values(self).get_result(conn)?;
 
             insert_into(versions_published_by::table)