Skip to content

Commit 97e55d3

Browse files
sgrifsgrif
committed
Don't parse Version#num for reads
On the majority of endpoints involving versions, we're parsing the version (using `.unwrap` even, which could take down the server) only to immediately convert it back into a string. This causes needless allocations, and a larger surface for errors to appear. We really only care about semver when ensuring a version is valid, and when determining the max version for a crate. Everywhere else, we can just use a String.
1 parent 35d2240 commit 97e55d3

File tree

7 files changed

+34
-52
lines changed

7 files changed

+34
-52
lines changed

Diff for: ‎src/bin/delete-version.rs

-2
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
extern crate cargo_registry;
1111
extern crate postgres;
1212
extern crate time;
13-
extern crate semver;
1413

1514
use std::env;
1615
use std::io;
@@ -38,7 +37,6 @@ fn delete(tx: &postgres::transaction::Transaction) {
3837
None => { println!("needs a version argument"); return }
3938
Some(s) => s,
4039
};
41-
let version = semver::Version::parse(&version).unwrap();
4240

4341
let krate = Crate::find_by_name(tx, &name).unwrap();
4442
let v = Version::find_by_num(tx, krate.id, &version).unwrap().unwrap();

Diff for: ‎src/bin/transfer-crates.rs

-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
extern crate cargo_registry;
99
extern crate postgres;
1010
extern crate time;
11-
extern crate semver;
1211

1312
use std::env;
1413
use std::io;

Diff for: ‎src/git.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ use std::fs::{self, File};
44
use std::io::prelude::*;
55
use std::path::{Path, PathBuf};
66

7-
use semver;
87
use git2;
98
use rustc_serialize::json;
109

@@ -69,7 +68,7 @@ pub fn add_crate(app: &App, krate: &Crate) -> CargoResult<()> {
6968
})
7069
}
7170

72-
pub fn yank(app: &App, krate: &str, version: &semver::Version,
71+
pub fn yank(app: &App, krate: &str, version: &str,
7372
yanked: bool) -> CargoResult<()> {
7473
let repo = app.git_repo.lock().unwrap();
7574
let repo = &*repo;

Diff for: ‎src/krate.rs

+10-9
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use std::io::prelude::*;
55
use std::io;
66
use std::mem;
77
use std::sync::Arc;
8+
use std::borrow::Cow;
89

910
use conduit::{Request, Response};
1011
use conduit_router::RequestParams;
@@ -64,7 +65,7 @@ pub struct EncodableCrate {
6465
pub badges: Option<Vec<EncodableBadge>>,
6566
pub created_at: String,
6667
pub downloads: i32,
67-
pub max_version: String,
68+
pub max_version: Cow<'static, str>,
6869
pub description: Option<String>,
6970
pub homepage: Option<String>,
7071
pub documentation: Option<String>,
@@ -232,13 +233,13 @@ impl Crate {
232233
}
233234

234235
pub fn minimal_encodable(self,
235-
max_version: semver::Version,
236+
max_version: Cow<'static, str>,
236237
badges: Option<Vec<Badge>>) -> EncodableCrate {
237238
self.encodable(max_version, None, None, None, badges)
238239
}
239240

240241
pub fn encodable(self,
241-
max_version: semver::Version,
242+
max_version: Cow<'static, str>,
242243
versions: Option<Vec<i32>>,
243244
keywords: Option<&[Keyword]>,
244245
categories: Option<&[Category]>,
@@ -268,7 +269,7 @@ impl Crate {
268269
keywords: keyword_ids,
269270
categories: category_ids,
270271
badges: badges,
271-
max_version: max_version.to_string(),
272+
max_version: max_version,
272273
documentation: documentation,
273274
homepage: homepage,
274275
description: description,
@@ -283,14 +284,14 @@ impl Crate {
283284
}
284285
}
285286

286-
pub fn max_version(&self, conn: &GenericConnection) -> CargoResult<semver::Version> {
287+
pub fn max_version(&self, conn: &GenericConnection) -> CargoResult<Cow<'static, str>> {
287288
let stmt = conn.prepare("SELECT num FROM versions WHERE crate_id = $1
288289
AND yanked = 'f'")?;
289290
let rows = stmt.query(&[&self.id])?;
290291
Ok(rows.iter()
291-
.map(|r| semver::Version::parse(&r.get::<_, String>("num")).unwrap())
292-
.max()
293-
.unwrap_or_else(|| semver::Version::parse("0.0.0").unwrap()))
292+
.map(|r| Cow::Owned(r.get("num")))
293+
.max_by_key(|v| semver::Version::parse(&v).ok())
294+
.unwrap_or_else(|| Cow::Borrowed("0.0.0")))
294295
}
295296

296297
pub fn versions(&self, conn: &GenericConnection) -> CargoResult<Vec<Version>> {
@@ -388,7 +389,7 @@ impl Crate {
388389
features: &HashMap<String, Vec<String>>,
389390
authors: &[String])
390391
-> CargoResult<Version> {
391-
match Version::find_by_num(conn, self.id, ver)? {
392+
match Version::find_by_num(conn, self.id, &ver.to_string())? {
392393
Some(..) => {
393394
return Err(human(format!("crate version `{}` is already uploaded",
394395
ver)))

Diff for: ‎src/tests/all.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -216,10 +216,10 @@ fn mock_user(req: &mut Request, u: User) -> User {
216216
}
217217

218218
fn mock_crate(req: &mut Request, krate: Crate) -> (Crate, Version) {
219-
mock_crate_vers(req, krate, &semver::Version::parse("1.0.0").unwrap())
219+
mock_crate_vers(req, krate, "1.0.0")
220220
}
221221

222-
fn mock_crate_vers(req: &mut Request, krate: Crate, v: &semver::Version)
222+
fn mock_crate_vers(req: &mut Request, krate: Crate, v: &str)
223223
-> (Crate, Version) {
224224
let user = req.extensions().find::<User>().unwrap();
225225
let mut krate = Crate::find_or_insert(req.tx().unwrap(), &krate.name,
@@ -231,7 +231,8 @@ fn mock_crate_vers(req: &mut Request, krate: Crate, v: &semver::Version)
231231
&krate.license,
232232
&None,
233233
krate.max_upload_size).unwrap();
234-
let v = krate.add_version(req.tx().unwrap(), v, &HashMap::new(), &[]);
234+
let v = semver::Version::parse(v).unwrap();
235+
let v = krate.add_version(req.tx().unwrap(), &v, &HashMap::new(), &[]);
235236
(krate, v.unwrap())
236237
}
237238

Diff for: ‎src/tests/krate.rs

+16-27
Original file line numberDiff line numberDiff line change
@@ -1156,14 +1156,12 @@ fn ignored_badges() {
11561156
fn reverse_dependencies() {
11571157
let (_b, app, middle) = ::app();
11581158

1159-
let v100 = semver::Version::parse("1.0.0").unwrap();
1160-
let v110 = semver::Version::parse("1.1.0").unwrap();
11611159
let mut req = ::req(app, Method::Get,
11621160
"/api/v1/crates/c1/reverse_dependencies");
11631161
::mock_user(&mut req, ::user("foo"));
1164-
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100);
1165-
let (_, c2v1) = ::mock_crate_vers(&mut req, ::krate("c2"), &v100);
1166-
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), &v110);
1162+
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), "1.0.0");
1163+
let (_, c2v1) = ::mock_crate_vers(&mut req, ::krate("c2"), "1.0.0");
1164+
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), "1.1.0");
11671165

11681166
::mock_dep(&mut req, &c2v1, &c1, None);
11691167
::mock_dep(&mut req, &c2v2, &c1, None);
@@ -1187,15 +1185,12 @@ fn reverse_dependencies() {
11871185
fn reverse_dependencies_when_old_version_doesnt_depend_but_new_does() {
11881186
let (_b, app, middle) = ::app();
11891187

1190-
let v100 = semver::Version::parse("1.0.0").unwrap();
1191-
let v110 = semver::Version::parse("1.1.0").unwrap();
1192-
let v200 = semver::Version::parse("2.0.0").unwrap();
11931188
let mut req = ::req(app, Method::Get,
11941189
"/api/v1/crates/c1/reverse_dependencies");
11951190
::mock_user(&mut req, ::user("foo"));
1196-
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v110);
1197-
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v100);
1198-
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), &v200);
1191+
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), "1.1.0");
1192+
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), "1.0.0");
1193+
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), "2.0.0");
11991194

12001195
::mock_dep(&mut req, &c2v2, &c1, None);
12011196

@@ -1210,14 +1205,12 @@ fn reverse_dependencies_when_old_version_doesnt_depend_but_new_does() {
12101205
fn reverse_dependencies_when_old_version_depended_but_new_doesnt() {
12111206
let (_b, app, middle) = ::app();
12121207

1213-
let v100 = semver::Version::parse("1.0.0").unwrap();
1214-
let v200 = semver::Version::parse("2.0.0").unwrap();
12151208
let mut req = ::req(app, Method::Get,
12161209
"/api/v1/crates/c1/reverse_dependencies");
12171210
::mock_user(&mut req, ::user("foo"));
1218-
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100);
1219-
let (_, c2v1) = ::mock_crate_vers(&mut req, ::krate("c2"), &v100);
1220-
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v200);
1211+
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), "1.0.0");
1212+
let (_, c2v1) = ::mock_crate_vers(&mut req, ::krate("c2"), "1.0.0");
1213+
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), "2.0.0");
12211214

12221215
::mock_dep(&mut req, &c2v1, &c1, None);
12231216

@@ -1231,15 +1224,13 @@ fn reverse_dependencies_when_old_version_depended_but_new_doesnt() {
12311224
fn prerelease_versions_not_included_in_reverse_dependencies() {
12321225
let (_b, app, middle) = ::app();
12331226

1234-
let v100 = semver::Version::parse("1.0.0").unwrap();
1235-
let v110_pre = semver::Version::parse("1.1.0-pre").unwrap();
12361227
let mut req = ::req(app, Method::Get,
12371228
"/api/v1/crates/c1/reverse_dependencies");
12381229
::mock_user(&mut req, ::user("foo"));
1239-
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100);
1240-
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v110_pre);
1241-
let (_, c3v1) = ::mock_crate_vers(&mut req, ::krate("c3"), &v100);
1242-
let _ = ::mock_crate_vers(&mut req, ::krate("c3"), &v110_pre);
1230+
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), "1.0.0");
1231+
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), "1.1.0-pre");
1232+
let (_, c3v1) = ::mock_crate_vers(&mut req, ::krate("c3"), "1.0.0");
1233+
let _ = ::mock_crate_vers(&mut req, ::krate("c3"), "1.1.0-pre");
12431234

12441235
::mock_dep(&mut req, &c3v1, &c1, None);
12451236

@@ -1254,14 +1245,12 @@ fn prerelease_versions_not_included_in_reverse_dependencies() {
12541245
fn yanked_versions_not_included_in_reverse_dependencies() {
12551246
let (_b, app, middle) = ::app();
12561247

1257-
let v100 = semver::Version::parse("1.0.0").unwrap();
1258-
let v200 = semver::Version::parse("2.0.0").unwrap();
12591248
let mut req = ::req(app, Method::Get,
12601249
"/api/v1/crates/c1/reverse_dependencies");
12611250
::mock_user(&mut req, ::user("foo"));
1262-
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), &v100);
1263-
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), &v100);
1264-
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), &v200);
1251+
let (c1, _) = ::mock_crate_vers(&mut req, ::krate("c1"), "1.0.0");
1252+
let _ = ::mock_crate_vers(&mut req, ::krate("c2"), "1.0.0");
1253+
let (_, c2v2) = ::mock_crate_vers(&mut req, ::krate("c2"), "2.0.0");
12651254

12661255
::mock_dep(&mut req, &c2v2, &c1, None);
12671256

Diff for: ‎src/version.rs

+3-8
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ use util::{RequestUtils, CargoResult, ChainError, internal, human};
2525
pub struct Version {
2626
pub id: i32,
2727
pub crate_id: i32,
28-
pub num: semver::Version,
28+
pub num: String,
2929
pub updated_at: Timespec,
3030
pub created_at: Timespec,
3131
pub downloads: i32,
@@ -61,9 +61,8 @@ pub struct VersionLinks {
6161
impl Version {
6262
pub fn find_by_num(conn: &GenericConnection,
6363
crate_id: i32,
64-
num: &semver::Version)
64+
num: &str)
6565
-> CargoResult<Option<Version>> {
66-
let num = num.to_string();
6766
let stmt = conn.prepare("SELECT * FROM versions \
6867
WHERE crate_id = $1 AND num = $2")?;
6968
let rows = stmt.query(&[&crate_id, &num])?;
@@ -192,15 +191,14 @@ impl Version {
192191

193192
impl Model for Version {
194193
fn from_row(row: &Row) -> Version {
195-
let num: String = row.get("num");
196194
let features: Option<String> = row.get("features");
197195
let features = features.map(|s| {
198196
json::decode(&s).unwrap()
199197
}).unwrap_or_else(|| HashMap::new());
200198
Version {
201199
id: row.get("id"),
202200
crate_id: row.get("crate_id"),
203-
num: semver::Version::parse(&num).unwrap(),
201+
num: row.get("num"),
204202
updated_at: row.get("updated_at"),
205203
created_at: row.get("created_at"),
206204
downloads: row.get("downloads"),
@@ -271,9 +269,6 @@ pub fn show(req: &mut Request) -> CargoResult<Response> {
271269
fn version_and_crate(req: &mut Request) -> CargoResult<(Version, Crate)> {
272270
let crate_name = &req.params()["crate_id"];
273271
let semver = &req.params()["version"];
274-
let semver = semver::Version::parse(semver).map_err(|_| {
275-
human(format!("invalid semver: {}", semver))
276-
})?;
277272
let tx = req.tx()?;
278273
let krate = Crate::find_by_name(tx, crate_name)?;
279274
let version = Version::find_by_num(tx, krate.id, &semver)?;

0 commit comments

Comments
 (0)