Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 44 additions & 21 deletions src/controllers/krate/publish.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another approach would be to calculate from all versions. Since TopVersion already needs to calculate from all versions, we could consider pre-sorting the versions and providing methodologies for TopVersion and default_versions to determine values from the sorted versions.

Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
use url::Url;

use crate::models::{
insert_version_owner_action, Category, Crate, DependencyKind, Keyword, NewCrate, NewVersion,
Rights, VersionAction,
default_versions::Version as DefaultVersion, insert_version_owner_action, Category, Crate,
DependencyKind, Keyword, NewCrate, NewVersion, Rights, VersionAction,
};

use crate::licenses::parse_license_expr;
Expand Down Expand Up @@ -64,7 +64,7 @@

Crate::validate_crate_name("crate", &metadata.name).map_err(bad_request)?;

let version = match semver::Version::parse(&metadata.vers) {
let semver = match semver::Version::parse(&metadata.vers) {
Ok(parsed) => parsed,
Err(_) => {
return Err(bad_request(format_args!(
Expand All @@ -75,7 +75,7 @@
};

// Convert the version back to a string to deal with any inconsistencies
let version_string = version.to_string();
let version_string = semver.to_string();

let request_log = req.request_log();
request_log.add("crate_name", &*metadata.name);
Expand Down Expand Up @@ -385,17 +385,46 @@
// Link this new version to all dependencies
add_dependencies(conn, &deps, version.id)?;

// Insert the default version if it doesn't already exist. Compared
// to only using a background job, this prevents us from getting
// into a situation where a crate exists in the `crates` table but
// doesn't have a default version in the `default_versions` table.
let inserted_default_versions = diesel::insert_into(default_versions::table)
.values((
default_versions::crate_id.eq(krate.id),
default_versions::version_id.eq(version.id),
))
.on_conflict_do_nothing()
.execute(conn)?;
let existing_default_version = default_versions::table
.inner_join(versions::table)
.filter(default_versions::crate_id.eq(krate.id))
.select(DefaultVersion::as_select())
.first(conn)
.optional()?;

// Upsert the `default_value` determined by the existing `default_value` and the
// published version. Note that this could potentially write an outdated version
// (although this should not happen regularly), as we might be comparing to an
// outdated value.
//
// Compared to only using a background job, this prevents us from getting into a
// situation where a crate exists in the `crates` table but doesn't have a default
// version in the `default_versions` table.
if let Some(existing_default_version) = existing_default_version {
let published_default_version = DefaultVersion {
id: version.id,
num: semver,
yanked: false,
};

if existing_default_version < published_default_version {
diesel::update(default_versions::table)
.filter(default_versions::crate_id.eq(krate.id))
.set(default_versions::version_id.eq(version.id))
.execute(conn)?;
}

Check warning on line 415 in src/controllers/krate/publish.rs

View check run for this annotation

Codecov / codecov/patch

src/controllers/krate/publish.rs#L415

Added line #L415 was not covered by tests

// Update the default version asynchronously in a background job
// to ensure correctness and eventual consistency.
UpdateDefaultVersion::new(krate.id).enqueue(conn)?;
} else {
diesel::insert_into(default_versions::table)
.values((
default_versions::crate_id.eq(krate.id),
default_versions::version_id.eq(version.id),
))
.execute(conn)?;
}

// Update all keywords for this crate
Keyword::update_crate(conn, &krate, &keywords)?;
Expand Down Expand Up @@ -446,12 +475,6 @@

SendPublishNotificationsJob::new(version.id).enqueue(conn)?;

// If this is a new version for an existing crate it is sufficient
// to update the default version asynchronously in a background job.
if inserted_default_versions == 0 {
UpdateDefaultVersion::new(krate.id).enqueue(conn)?;
}

// Experiment: check new crates for potential typosquatting.
if existing_crate.is_none() {
CheckTyposquat::new(&krate.name).enqueue(conn)?;
Expand Down
2 changes: 1 addition & 1 deletion src/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub mod helpers;
mod action;
pub mod category;
mod crate_owner_invitation;
mod default_versions;
pub mod default_versions;
pub mod dependency;
mod download;
mod email;
Expand Down
8 changes: 4 additions & 4 deletions src/models/default_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ use diesel::prelude::*;
#[derive(Clone, Debug, PartialEq, Eq, Queryable, Selectable)]
#[diesel(table_name = versions)]
#[diesel(check_for_backend(diesel::pg::Pg))]
struct Version {
id: i32,
pub struct Version {
pub id: i32,
#[diesel(deserialize_as = SemverVersion)]
num: semver::Version,
yanked: bool,
pub num: semver::Version,
pub yanked: bool,
}

impl Version {
Expand Down