Skip to content

Commit a2d93a1

Browse files
sgrifsgrif
committed
Fix reverse_dependencies
So in rust-lang#592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert rust-lang#592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes rust-lang#602.
1 parent 52777af commit a2d93a1

File tree

4 files changed

+131
-11
lines changed

4 files changed

+131
-11
lines changed

Diff for: ‎migrations/20170308140537_create_to_semver_no_prerelease/down.sql

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
DROP INDEX versions_to_semver_no_prerelease_idx;
2+
DROP FUNCTION to_semver_no_prerelease(text);
3+
DROP TYPE semver_triple;

Diff for: ‎migrations/20170308140537_create_to_semver_no_prerelease/up.sql

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
CREATE TYPE semver_triple AS (
2+
major int4,
3+
minor int4,
4+
teeny int4
5+
);
6+
7+
CREATE FUNCTION to_semver_no_prerelease(text) RETURNS semver_triple IMMUTABLE AS $$
8+
SELECT (
9+
split_part($1, '.', 1)::int4,
10+
split_part($1, '.', 2)::int4,
11+
split_part(split_part($1, '+', 1), '.', 3)::int4
12+
)::semver_triple
13+
WHERE strpos($1, '-') = 0
14+
$$ LANGUAGE SQL
15+
;
16+
17+
CREATE INDEX versions_to_semver_no_prerelease_idx ON versions (crate_id, to_semver_no_prerelease(num) DESC NULLS LAST) WHERE NOT yanked;

Diff for: ‎src/krate.rs

+15-11
Original file line numberDiff line numberDiff line change
@@ -432,32 +432,36 @@ impl Crate {
432432
-> CargoResult<(Vec<(Dependency, String, i32)>, i64)> {
433433
let select_sql = "
434434
FROM dependencies
435-
INNER JOIN versions
435+
INNER JOIN (
436+
SELECT versions.*,
437+
row_number() OVER (PARTITION BY crate_id ORDER BY to_semver_no_prerelease(num) DESC NULLS LAST) rn
438+
FROM versions
439+
WHERE NOT yanked
440+
) versions
436441
ON versions.id = dependencies.version_id
442+
AND rn = 1
437443
INNER JOIN crates
438-
ON crates.id = versions.crate_id
444+
ON versions.crate_id = crates.id
439445
WHERE dependencies.crate_id = $1
440-
AND versions.num = $2
441446
";
442-
let fetch_sql = format!("SELECT DISTINCT ON (crate_downloads, crate_name)
447+
let fetch_sql = format!("SELECT DISTINCT ON (crate_downloads, crates.id)
443448
dependencies.*,
444449
crates.downloads AS crate_downloads,
445450
crates.name AS crate_name
446451
{}
447452
ORDER BY crate_downloads DESC
448-
OFFSET $3
449-
LIMIT $4",
450-
select_sql);
451-
let count_sql = format!("SELECT COUNT(DISTINCT(crates.id)) {}", select_sql);
453+
OFFSET $2
454+
LIMIT $3",
455+
select_sql);
456+
let count_sql = format!("SELECT COUNT(DISTINCT crates.*) {}", select_sql);
452457

453458
let stmt = conn.prepare(&fetch_sql)?;
454-
let max_version = self.max_version(conn)?.to_string();
455-
let vec: Vec<_> = stmt.query(&[&self.id, &max_version, &offset, &limit])?
459+
let vec: Vec<_> = stmt.query(&[&self.id, &offset, &limit])?
456460
.iter()
457461
.map(|r| (Model::from_row(&r), r.get("crate_name"), r.get("crate_downloads")))
458462
.collect();
459463
let stmt = conn.prepare(&count_sql)?;
460-
let cnt: i64 = stmt.query(&[&self.id, &max_version])?.iter().next().unwrap().get(0);
464+
let cnt: i64 = stmt.query(&[&self.id])?.iter().next().unwrap().get(0);
461465

462466
Ok((vec, cnt))
463467
}

Diff for: ‎src/tests/krate.rs

+96
Original file line numberDiff line numberDiff line change
@@ -1183,6 +1183,102 @@ fn reverse_dependencies() {
11831183
assert_eq!(deps.meta.total, 0);
11841184
}
11851185

1186+
#[test]
1187+
fn reverse_dependencies_when_old_version_doesnt_depend_but_new_does() {
1188+
let (_b, app, middle) = ::app();
1189+
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();
1193+
let mut req = ::req(app, Method::Get,
1194+
"/api/v1/crates/c1/reverse_dependencies");
1195+
::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);
1199+
1200+
::mock_dep(&mut req, &c2v2, &c1, None);
1201+
1202+
let mut response = ok_resp!(middle.call(&mut req));
1203+
let deps = ::json::<RevDeps>(&mut response);
1204+
assert_eq!(deps.dependencies.len(), 1);
1205+
assert_eq!(deps.meta.total, 1);
1206+
assert_eq!(deps.dependencies[0].crate_id, "c2");
1207+
}
1208+
1209+
#[test]
1210+
fn reverse_dependencies_when_old_version_depended_but_new_doesnt() {
1211+
let (_b, app, middle) = ::app();
1212+
1213+
let v100 = semver::Version::parse("1.0.0").unwrap();
1214+
let v200 = semver::Version::parse("2.0.0").unwrap();
1215+
let mut req = ::req(app, Method::Get,
1216+
"/api/v1/crates/c1/reverse_dependencies");
1217+
::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);
1221+
1222+
::mock_dep(&mut req, &c2v1, &c1, None);
1223+
1224+
let mut response = ok_resp!(middle.call(&mut req));
1225+
let deps = ::json::<RevDeps>(&mut response);
1226+
assert_eq!(deps.dependencies.len(), 0);
1227+
assert_eq!(deps.meta.total, 0);
1228+
}
1229+
1230+
#[test]
1231+
fn prerelease_versions_not_included_in_reverse_dependencies() {
1232+
let (_b, app, middle) = ::app();
1233+
1234+
let v100 = semver::Version::parse("1.0.0").unwrap();
1235+
let v110_pre = semver::Version::parse("1.1.0-pre").unwrap();
1236+
let mut req = ::req(app, Method::Get,
1237+
"/api/v1/crates/c1/reverse_dependencies");
1238+
::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);
1243+
1244+
::mock_dep(&mut req, &c3v1, &c1, None);
1245+
1246+
let mut response = ok_resp!(middle.call(&mut req));
1247+
let deps = ::json::<RevDeps>(&mut response);
1248+
assert_eq!(deps.dependencies.len(), 1);
1249+
assert_eq!(deps.meta.total, 1);
1250+
assert_eq!(deps.dependencies[0].crate_id, "c3");
1251+
}
1252+
1253+
#[test]
1254+
fn yanked_versions_not_included_in_reverse_dependencies() {
1255+
let (_b, app, middle) = ::app();
1256+
1257+
let v100 = semver::Version::parse("1.0.0").unwrap();
1258+
let v200 = semver::Version::parse("2.0.0").unwrap();
1259+
let mut req = ::req(app, Method::Get,
1260+
"/api/v1/crates/c1/reverse_dependencies");
1261+
::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);
1265+
1266+
::mock_dep(&mut req, &c2v2, &c1, None);
1267+
1268+
let mut response = ok_resp!(middle.call(&mut req));
1269+
let deps = ::json::<RevDeps>(&mut response);
1270+
assert_eq!(deps.dependencies.len(), 1);
1271+
assert_eq!(deps.meta.total, 1);
1272+
assert_eq!(deps.dependencies[0].crate_id, "c2");
1273+
1274+
t!(c2v2.yank(req.tx().unwrap(), true));
1275+
1276+
let mut response = ok_resp!(middle.call(&mut req));
1277+
let deps = ::json::<RevDeps>(&mut response);
1278+
assert_eq!(deps.dependencies.len(), 0);
1279+
assert_eq!(deps.meta.total, 0);
1280+
}
1281+
11861282
#[test]
11871283
fn author_license_and_description_required() {
11881284
let (_b, app, middle) = ::app();

0 commit comments

Comments
 (0)