Skip to content

Commit a3d6059

Browse files
committed
models/version: Rely on UniqueViolation database error for "version already exists" response
Previously, it was theoretically possible to publish two versions with the same base version number, but different build metadata, at the same time due to a race condition between the check and the insert. This commit fixes the issue by instead relying on the database uniqueness error to build the appropriate response.
1 parent 987cda3 commit a3d6059

File tree

1 file changed

+17
-17
lines changed

1 file changed

+17
-17
lines changed

src/models/version.rs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use crate::util::errors::{bad_request, AppResult};
1010

1111
use crate::models::{Crate, Dependency, User};
1212
use crate::schema::*;
13-
use crate::sql::split_part;
1413
use crate::util::diesel::Conn;
1514

1615
// Queryable has a custom implementation below
@@ -104,24 +103,20 @@ pub struct NewVersion<'a> {
104103

105104
impl NewVersion<'_> {
106105
pub fn save(&self, conn: &mut impl Conn, published_by_email: &str) -> AppResult<Version> {
107-
use diesel::dsl::exists;
108-
use diesel::{insert_into, select};
106+
use diesel::insert_into;
109107

110108
conn.transaction(|conn| {
111-
let num_no_build = strip_build_metadata(self.num);
112-
113-
let already_uploaded = versions::table
114-
.filter(versions::crate_id.eq(self.crate_id))
115-
.filter(split_part(versions::num, "+", 1).eq(num_no_build));
116-
117-
if select(exists(already_uploaded)).get_result(conn)? {
118-
return Err(bad_request(format_args!(
119-
"crate version `{}` is already uploaded",
120-
num_no_build
121-
)));
122-
}
123-
124-
let version: Version = insert_into(versions::table).values(self).get_result(conn)?;
109+
let version: Version = match insert_into(versions::table).values(self).get_result(conn)
110+
{
111+
Err(error) if is_unique_violation(&error) => {
112+
return Err(bad_request(format_args!(
113+
"crate version `{}` is already uploaded",
114+
self.num_no_build
115+
)));
116+
}
117+
Err(error) => return Err(error.into()),
118+
Ok(version) => version,
119+
};
125120

126121
insert_into(versions_published_by::table)
127122
.values((
@@ -141,6 +136,11 @@ fn strip_build_metadata(version: &str) -> &str {
141136
.unwrap_or(version)
142137
}
143138

139+
fn is_unique_violation(error: &diesel::result::Error) -> bool {
140+
use diesel::result::{DatabaseErrorKind::UniqueViolation, Error};
141+
matches!(error, Error::DatabaseError(UniqueViolation, _))
142+
}
143+
144144
/// The highest version (semver order) and the most recently updated version.
145145
/// Typically used for a single crate.
146146
#[derive(Debug, Clone, Eq, PartialEq)]

0 commit comments

Comments
 (0)