From aa363b64b7e7482eeb57f27ff33539e783282908 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 23 May 2023 15:50:34 +0200 Subject: [PATCH 1/2] sql: Make `split_part()` available in Diesel queries --- src/sql.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sql.rs b/src/sql.rs index 751c4936d2..9879b308e8 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -1,4 +1,4 @@ -use diesel::sql_types::{Date, Double, Interval, SingleValue, Text, Timestamp}; +use diesel::sql_types::{Date, Double, Integer, Interval, SingleValue, Text, Timestamp}; sql_function!(#[aggregate] fn array_agg(x: T) -> Array); sql_function!(fn canon_crate_name(x: Text) -> Text); @@ -12,3 +12,4 @@ sql_function! { sql_function!(fn floor(x: Double) -> Integer); sql_function!(fn greatest(x: T, y: T) -> T); sql_function!(fn least(x: T, y: T) -> T); +sql_function!(fn split_part(string: Text, delimiter: Text, n: Integer) -> Text); From 456fb9bc51b1bd223ebf357c02ec1207b688ef3d Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 23 May 2023 16:09:30 +0200 Subject: [PATCH 2/2] Prevent similar versions that only differ in build metadata from being published `cargo` does not handle these situations particularly well and the semver specification is a little ambiguous about how to handle build metadata. We still have roughly 600 problematic versions in the database, so we can't introduce a unique index yet. --- src/models/version.rs | 18 ++++-- ...publish_version_with_build_metadata_1.json | 62 +++++++++++++++++++ ...publish_version_with_build_metadata_2.json | 62 +++++++++++++++++++ ...publish_version_with_build_metadata_3.json | 62 +++++++++++++++++++ src/tests/krate/publish.rs | 52 ++++++++++++++++ ..._with_build_metadata@build_metadata_1.snap | 35 +++++++++++ ..._with_build_metadata@build_metadata_2.snap | 35 +++++++++++ ..._with_build_metadata@build_metadata_3.snap | 35 +++++++++++ 8 files changed, 357 insertions(+), 4 deletions(-) create mode 100644 src/tests/http-data/krate_publish_version_with_build_metadata_1.json create mode 100644 src/tests/http-data/krate_publish_version_with_build_metadata_2.json create mode 100644 src/tests/http-data/krate_publish_version_with_build_metadata_3.json create mode 100644 src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_1.snap create mode 100644 src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_2.snap create mode 100644 src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_3.snap diff --git a/src/models/version.rs b/src/models/version.rs index 54315e7d67..49215c6866 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -7,6 +7,7 @@ use crate::util::errors::{cargo_err, AppResult}; use crate::models::{Crate, Dependency, User}; use crate::schema::*; +use crate::sql::split_part; // Queryable has a custom implementation below #[derive(Clone, Identifiable, Associations, Debug, Queryable, Deserialize, Serialize)] @@ -168,14 +169,16 @@ impl NewVersion { use diesel::{insert_into, select}; conn.transaction(|conn| { + let num_no_build = strip_build_metadata(&self.num); + let already_uploaded = versions .filter(crate_id.eq(self.crate_id)) - .filter(num.eq(&self.num)); + .filter(split_part(num, "+", 1).eq(num_no_build)); + if select(exists(already_uploaded)).get_result(conn)? { return Err(cargo_err(&format_args!( - "crate version `{}` is already \ - uploaded", - self.num + "crate version `{}` is already uploaded", + num_no_build ))); } @@ -219,6 +222,13 @@ fn validate_license_expr(s: &str) -> AppResult<()> { Ok(()) } +fn strip_build_metadata(version: &str) -> &str { + version + .split_once('+') + .map(|parts| parts.0) + .unwrap_or(version) +} + #[cfg(test)] mod tests { use super::{validate_license_expr, TopVersions}; diff --git a/src/tests/http-data/krate_publish_version_with_build_metadata_1.json b/src/tests/http-data/krate_publish_version_with_build_metadata_1.json new file mode 100644 index 0000000000..3b1b249ac2 --- /dev/null +++ b/src/tests/http-data/krate_publish_version_with_build_metadata_1.json @@ -0,0 +1,62 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/foo/foo-1.0.0+foo.crate", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "35" + ], + [ + "content-type", + "application/gzip" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/3/f/foo", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "148" + ], + [ + "content-type", + "text/plain" + ] + ], + "body": "{\"name\":\"foo\",\"vers\":\"1.0.0+foo\",\"deps\":[],\"cksum\":\"acb5604b126ac894c1eb11c4575bf2072fea61232a888e453770c79d7ed56419\",\"features\":{},\"yanked\":false}\n" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + } +] diff --git a/src/tests/http-data/krate_publish_version_with_build_metadata_2.json b/src/tests/http-data/krate_publish_version_with_build_metadata_2.json new file mode 100644 index 0000000000..2b4d0a7f50 --- /dev/null +++ b/src/tests/http-data/krate_publish_version_with_build_metadata_2.json @@ -0,0 +1,62 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/foo/foo-1.0.0-beta.1.crate", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "35" + ], + [ + "content-type", + "application/gzip" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/3/f/foo", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "151" + ], + [ + "content-type", + "text/plain" + ] + ], + "body": "{\"name\":\"foo\",\"vers\":\"1.0.0-beta.1\",\"deps\":[],\"cksum\":\"acb5604b126ac894c1eb11c4575bf2072fea61232a888e453770c79d7ed56419\",\"features\":{},\"yanked\":false}\n" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + } +] diff --git a/src/tests/http-data/krate_publish_version_with_build_metadata_3.json b/src/tests/http-data/krate_publish_version_with_build_metadata_3.json new file mode 100644 index 0000000000..3b1b249ac2 --- /dev/null +++ b/src/tests/http-data/krate_publish_version_with_build_metadata_3.json @@ -0,0 +1,62 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/foo/foo-1.0.0+foo.crate", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "35" + ], + [ + "content-type", + "application/gzip" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/3/f/foo", + "method": "PUT", + "headers": [ + [ + "accept", + "*/*" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-length", + "148" + ], + [ + "content-type", + "text/plain" + ] + ], + "body": "{\"name\":\"foo\",\"vers\":\"1.0.0+foo\",\"deps\":[],\"cksum\":\"acb5604b126ac894c1eb11c4575bf2072fea61232a888e453770c79d7ed56419\",\"features\":{},\"yanked\":false}\n" + }, + "response": { + "status": 200, + "headers": [], + "body": "" + } + } +] diff --git a/src/tests/krate/publish.rs b/src/tests/krate/publish.rs index 82f9759a6f..28fc40eca8 100644 --- a/src/tests/krate/publish.rs +++ b/src/tests/krate/publish.rs @@ -1,5 +1,6 @@ use crate::builders::{CrateBuilder, DependencyBuilder, PublishBuilder}; use crate::new_category; +use crate::util::insta::assert_yaml_snapshot; use crate::util::{RequestHelper, TestApp}; use cargo_registry::controllers::krate::publish::{ missing_metadata_error_message, MISSING_RIGHTS_ERROR_MESSAGE, WILDCARD_ERROR_MESSAGE, @@ -1035,3 +1036,54 @@ fn empty_payload() { json!({ "errors": [{ "detail": "invalid metadata length" }] }) ); } + +fn version_with_build_metadata(v1: &str, v2: &str, expected_error: &str) { + let (_app, _anon, _cookie, token) = TestApp::full().with_token(); + + let response = token.publish_crate(PublishBuilder::new("foo").version(v1)); + assert_eq!(response.status(), StatusCode::OK); + assert_yaml_snapshot!(response.into_json(), { + ".crate.created_at" => "[datetime]", + ".crate.updated_at" => "[datetime]", + }); + + let response = token.publish_crate(PublishBuilder::new("foo").version(v2)); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.into_json(), + json!({ "errors": [{ "detail": expected_error }] }) + ); +} + +#[test] +fn version_with_build_metadata_1() { + insta::with_settings!({ snapshot_suffix => "build_metadata_1" }, { + version_with_build_metadata( + "1.0.0+foo", + "1.0.0+bar", + "crate version `1.0.0` is already uploaded", + ); + }); +} + +#[test] +fn version_with_build_metadata_2() { + insta::with_settings!({ snapshot_suffix => "build_metadata_2" }, { + version_with_build_metadata( + "1.0.0-beta.1", + "1.0.0-beta.1+2", + "crate version `1.0.0-beta.1` is already uploaded", + ); + }); +} + +#[test] +fn version_with_build_metadata_3() { + insta::with_settings!({ snapshot_suffix => "build_metadata_3" }, { + version_with_build_metadata( + "1.0.0+foo", + "1.0.0", + "crate version `1.0.0` is already uploaded", + ); + }); +} diff --git a/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_1.snap b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_1.snap new file mode 100644 index 0000000000..47165e09a6 --- /dev/null +++ b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_1.snap @@ -0,0 +1,35 @@ +--- +source: src/tests/krate/publish.rs +expression: response.into_json() +--- +crate: + badges: ~ + categories: ~ + created_at: "[datetime]" + description: description + documentation: ~ + downloads: 0 + exact_match: false + homepage: ~ + id: foo + keywords: ~ + links: + owner_team: /api/v1/crates/foo/owner_team + owner_user: /api/v1/crates/foo/owner_user + owners: /api/v1/crates/foo/owners + reverse_dependencies: /api/v1/crates/foo/reverse_dependencies + version_downloads: /api/v1/crates/foo/downloads + versions: /api/v1/crates/foo/versions + max_stable_version: 1.0.0+foo + max_version: 1.0.0+foo + name: foo + newest_version: 1.0.0+foo + recent_downloads: ~ + repository: ~ + updated_at: "[datetime]" + versions: ~ +warnings: + invalid_badges: [] + invalid_categories: [] + other: [] + diff --git a/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_2.snap b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_2.snap new file mode 100644 index 0000000000..fa5634b686 --- /dev/null +++ b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_2.snap @@ -0,0 +1,35 @@ +--- +source: src/tests/krate/publish.rs +expression: response.into_json() +--- +crate: + badges: ~ + categories: ~ + created_at: "[datetime]" + description: description + documentation: ~ + downloads: 0 + exact_match: false + homepage: ~ + id: foo + keywords: ~ + links: + owner_team: /api/v1/crates/foo/owner_team + owner_user: /api/v1/crates/foo/owner_user + owners: /api/v1/crates/foo/owners + reverse_dependencies: /api/v1/crates/foo/reverse_dependencies + version_downloads: /api/v1/crates/foo/downloads + versions: /api/v1/crates/foo/versions + max_stable_version: ~ + max_version: 1.0.0-beta.1 + name: foo + newest_version: 1.0.0-beta.1 + recent_downloads: ~ + repository: ~ + updated_at: "[datetime]" + versions: ~ +warnings: + invalid_badges: [] + invalid_categories: [] + other: [] + diff --git a/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_3.snap b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_3.snap new file mode 100644 index 0000000000..47165e09a6 --- /dev/null +++ b/src/tests/krate/snapshots/all__krate__publish__version_with_build_metadata@build_metadata_3.snap @@ -0,0 +1,35 @@ +--- +source: src/tests/krate/publish.rs +expression: response.into_json() +--- +crate: + badges: ~ + categories: ~ + created_at: "[datetime]" + description: description + documentation: ~ + downloads: 0 + exact_match: false + homepage: ~ + id: foo + keywords: ~ + links: + owner_team: /api/v1/crates/foo/owner_team + owner_user: /api/v1/crates/foo/owner_user + owners: /api/v1/crates/foo/owners + reverse_dependencies: /api/v1/crates/foo/reverse_dependencies + version_downloads: /api/v1/crates/foo/downloads + versions: /api/v1/crates/foo/versions + max_stable_version: 1.0.0+foo + max_version: 1.0.0+foo + name: foo + newest_version: 1.0.0+foo + recent_downloads: ~ + repository: ~ + updated_at: "[datetime]" + versions: ~ +warnings: + invalid_badges: [] + invalid_categories: [] + other: [] +